import GAPGroups #84
import GAPGroups #84
Conversation
This comment has been minimized.
This comment has been minimized.
... to avoid clash with Hecke symbol of the same name
Codecov Report
@@ Coverage Diff @@
## master #84 +/- ##
===========================================
+ Coverage 12.37% 28.00% +15.63%
===========================================
Files 7 12 +5
Lines 1641 2282 +641
===========================================
+ Hits 203 639 +436
- Misses 1438 1643 +205
|
|
@GDeFranceschi there is a weird test failure in one of the Travis tests, not visible in the others, in |
|
@GDeFranceschi that test uses random elements. So perhaps it only fails "sometimes". I didn't check the mathematics behind that test -- there are zero comments. I am sure I could figure it out given enough time, but perhaps you could just add some comments to explain it, and at the same time verify it is really correct, mathematically? |
I omitted this aspect for simplicity, but this is part of why this solution is hacky, the structural members can be special cased (e.g. by using |
| end | ||
|
|
||
| function show(io::IO, x::GroupCoset) | ||
| if x.side == :right |
There was a problem hiding this comment.
a = GAP.gap_to_julia(GAP.Globals.StringView(x.H.X))
b = GAP.gap_to_julia(GAP.Globals.StringView(x.repr.X))
if x.side == :right
print(io, a, " * ", b)
else
print(io, b, " * ", a)
endis much more readable (at least to my eyes); You may also think about not using x.side directly, but to define side function for future maintainability
|
On Wed, Apr 29, 2020 at 02:01:33PM -0700, kalmarek wrote:
@kalmarek commented on this pull request.
> + else
+ return false, nothing
+ end
+end
+# END elements conjugation
+
+# START subgroups conjugation
+"""
+ conjugacy_class(G::T, H::T) where T<:Group -> GroupConjClass
+return the subgroup conjugacy class `cc` of `H` in `G`, where `H` = `representative`(`cc`).
+"""
+function conjugacy_class(G::T, g::T) where T<:Group
+ return _conjugacy_class(G, g, GAP.Globals.ConjugacyClassSubgroups(G.X,g.X))
+end
+
+function Base.rand(C::GroupConjClass{S,T}) where S where T<:Group
In this case I think GAP should use julias RNG. otherwise we need to
warn users to use `GAPseed!(...)` when they want to get a
deterministic result; But usage of GAP deep in some Oscar function is
implementation detail (users don't have to know that the underlying
functions call some randomized versions of algorithms in GAP) so users
will not know that they need to use it.
Partly agree: for reproducibility, it would be really good if, by
default, all of Oscar uses exactly one RNG for all purposes, so that
onely one set_seed is sufficient. I don't know about Polymake and
Singular, but Nemo (flint, gmp) is a fair way off this goal.
… --
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#84 (comment)
|
|
@fieker I have no idea how you would achieve that. You can't ask a C library like Flint to use a Julia RNG. I think the best you can hope for is a way to seed the Flint RNG's (plural) from Julia, for reproducibility. |
I'm not familiar with flint's RNG system, but there might be a way to use Julia's RNG if it's not too different from GMP's, with a POC at JuliaLang/julia#35111. That said, seeding Fllint RNGs from Julia might indeed be good enough. |
|
Well obviously you can't make Julia a dependency of Flint. Anyhow, Flint has multiple RNG's as the GMP one is too heavy duty for some applications, and just as a matter of principle, libraries shouldn't be assumed to have RNG's compatible with the programs that call them. If I did that for Julia, I'd have to do it for everyone who asked. I'm happy to add a facility to seed the Flint RNG's some day. There are many different kinds of RNG's though, and Flint may add additional ones in the future, and seeds vary depending on the type of RNG. You'd be better off just asking for a function to seed all generators in a fixed way, with fixed seeds hard coded into Flint that the user doesn't have to supply. Of course then you are going to have the problem that the same tests are run all the time with no variation, which doesn't help find bugs. So in that case you would be better asking for a function that returns an object which contains values which can be used to seed Flint in the same way in the future, without knowing what those seeds actually are. |
Sure, and similarly GMP doesn't depend on Julia. The mechanism is rather that GMP allows to provide your own C RNG, and Julia can provide a C interface to its RNG. But let's not hijack this already big PR, this is surely not urgent we can discuss it in more details somewhere else. |
|
On Fri, May 01, 2020 at 04:02:03AM -0700, wbhart wrote:
@fieker I have no idea how you would achieve that. You can't ask a C library like Flint to use a Julia RNG. I think the best you can hope for is a way to seed the Flint RNG's (plural) from Julia, for reproducibility.
I know all of the above, yet, in the long term, all random-sources in
Oscar should have a single status/ reset button. Otherwise,
reproducibility is completely out...
I'd not make it a priority now.
However, as I can tell flint to use my own (julia's) malloc, I see no
reason why I cannot do this for randomness.
… --
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#84 (comment)
|
|
Because malloc is a function and therefore has a standard interface. Randomness is a concept, for which there are many algorithms. There is no standard interface. I already said what the correct way to handle this would be. |
|
On Sun, May 03, 2020 at 11:19:07PM -0700, wbhart wrote:
Because malloc is a function and therefore has a standard interface. Randomness is a concept, for which there are many algorithms. There is no standard interface.
I already said what the correct way to handle this would be.
I disagree slightly, but your approach is easier to handle.
At the very least this would require, on startup, to seed everything
through julia
…
--
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#84 (comment)
|
|
Lots of things can go wrong with pseudorandom generators, e.g. they can for example decide to only return even numbers, or always with the top bits very similar or the bottom bits, or have very short periods, or be very non-uniform in their distribution, or have patterns in their output, odd even odd even. All of these things can and do go wrong if you take a naive approach. We don't need cryptographically secure random generation (until we do), but we certainly need to care that generators are seeded appropriately, whether that be with 4096 bits of randomness or a pair of words or a random double in a specific range with certain properties or a pair of random primes with a certain number of bits. You can only generate an appropriate seed for a given generator if you know some specifics about the generator. The idea that you can replace a given generator with any supplied generator, or seed a given generator with any old seed is simply not correct. We've hit this problem in Flint in the past, with things actually hanging because of poor quality random generation, which is precisely why we changed the way it's done. Imagine someone tries to do an experiment to evaluate a conjecture computationally in Oscar and the distribution they get is subtly altered because of poor quality random generation! |
|
On Mon, May 04, 2020 at 12:10:19AM -0700, wbhart wrote:
Lots of things can go wrong with pseudorandom generators, e.g. they
can for example decide to only return even numbers, or always with the
top bits very similar or the bottom bits, or have very short periods,
or be very non-uniform in their distribution, or have patterns in
their output, odd even odd even. All of these things can and do go
wrong if you take a naive approach.
well known
We don't need cryptographically secure random generation (until we
do), but we certainly need to care that generators are seeded
appropriately, whether that be with 4096 bits of randomness or a pair
of words or a random double in a specific range with certain
properties or a pair of random primes with a certain number of bits.
You can only generate an appropriate seed for a given generator if you
know some specifics about the generator. The idea that you can replace
a given generator with any supplied generator, or seed a given
generator with any old seed is simply not correct.
well known
We've hit this problem in Flint in the past, with things actually
hanging because of poor quality random generation, which is precisely
why we changed the way it's done.
Yep, encountered this before.
However, it might actually be that other generators are even better than
flints build-in. Otherwise, if flints is so much better than we should
try to make this one the oscar-default
Bottom line: for now, we cannot do anything (no manpower). Long-term, it
needs to be sorted, as, at least for debugging and development,
reproducibility is important, thus using >>1 independent sources of
random that are totally wild is difficult to support long term
…
Imagine someone tries to do an experiment to evaluate a conjecture computationally in Oscar and the distribution they get is subtly altered because of poor quality random generation!
--
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#84 (comment)
|
|
How am I not making myself clear here? There is no one way to do random generation! Even Flint doesn't have one random generator! And no, Flint's random generators absolutely suck eggs. There is an ever so slightly better set of random generators here: https://github.com/wbhart/bsdnt/tree/master/rand I helped with it, but the substantial stuff is by Brian Gladman, based on work of George Marsaglia. We discussed how to do it properly, but it would be a huge job. I vaguely recall this was a first attempt along that path. But we never brought it to completion: |
| SL(n::Int, q::Int) | ||
| return the special linear group of dimension `n` over the field GF(`q`). It is returned of type `T` for `T` = `MatrixGroup` or `PermGroup`. | ||
| """ | ||
| SL = special_linear_group |
There was a problem hiding this comment.
| SL = special_linear_group | |
| const SL = special_linear_group |
Also, why is this here, and not where special_linear_group is defined? And why the duplicate documentation?
The same of course holds for GL, Sp, etc.
|
@GDeFranceschi in order to be more systematic about your testing, you could take a look at the code coverage report for your branch here: https://codecov.io/gh/oscar-system/Oscar.jl/tree/GAPGroups/src/Groups For example, looking at https://codecov.io/gh/oscar-system/Oscar.jl/src/GAPGroups/src/Groups/group_constructors.jl indicates that |
| if n < 1 | ||
| throw(ArgumentError("n must be a positive integer")) | ||
| end | ||
| return T(GAP.Globals.SymmetricGroup(_gap_filter(T), n)) |
There was a problem hiding this comment.
I think it would make more sense if symmetric_group(n::Int64) was implemented by calling symmetric_group(::Type{T}, n::Int) , i.e. symmetric_group(n::Int64) = symmetric_group(PermGroup, n)
|
Thank you to everybody here for your comments. Please keep them coming! That said, we merged this PR for now -- not because the work is done or complete or perfect or anything like that, but simply because this way at least we can use the bits we have now in some basic demos etc. Note that there is no guarantee yet that anything will stay as it is -- this is still very much code in flux. @GDeFranceschi please go through the unresolved comments here; where it makes sense, either address them to your new PR (once it has been created), or transfer the comments to new comments there. |
The test for GAPGroups work on my laptop.