Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Some thoughts #3

Closed
fingolfin opened this issue Mar 8, 2021 · 0 comments · Fixed by #7
Closed

Some thoughts #3

fingolfin opened this issue Mar 8, 2021 · 0 comments · Fixed by #7

Comments

@fingolfin
Copy link
Collaborator

fingolfin commented Mar 8, 2021

Hi @kalmarek thank you very much for working on this. Some quick remarks from looking at the README only (so if the code or docstrings would have made things clearer, I missed that, but I didn't find a link to a generated version of the manual; is there one? perhaps add a badge for it in the README?):

  • It says "If finiteness can not be easily established one needs to override the default ..." and then talks about the default being "IteratorSize = Base.HasLength" -- but doesn't that default actually indicate that the default is that groups of a given type are all assumed to be finite and with order small enough to fit into an Int?? I'd really suggest to make the default SizeUnknown, and perhaps we can add a "trait" which makes it easy to specify that all instances of a given type are finite (but even then, it would mean that I can't use the iterator interface to get, say, the first 100 elements of Sym(100)). If you have good arguments why the default should really be HasLength, let's at least say something more like this (in spirit; the wording I use here probably is crap): "By default, HasLength is used which means that iteration will work for finite groups of your type, but will result in an error if the group has more than typemax(Int) elements"

  • from reading this text, I suspect we have divergent ideas about what hasgens(G) == true should mean: for me (coming from the GAP background) it means "generators of this group are already known / they are already set" (so it could also be called knowsgens(G); for you it seems to mean "generators are known or computable" (i.e. cancomputegens(G)) . At least that's how I interpret the explanation for GroupsCore.gens.... I any case, we need to agree on the intended meaning (possibly splitting the function into two or more, as needed), and carefully document this

  • BTW I am mostly open on the has vs knows vs ... discussion, but it ideally should be decided globally for all of OSCAR (this may very well require changing code). That said, I am happy to use one set of names now even if it means we need to change it down the road (i.e., IMHO we don't need to settle this with finality before we can proceed). Let me add, however, that for GAP, it was also useful to have a generic way to map from a "getter" to a "tester": i.e. given the "getter" function IsFinite, in GAP one can call Tester(IsFinite) and it will return the "tester" function HasIsFinite (which determines whether GAP already "knows" whether the group is finite. I.e., whether is information is cached or not)

  • what is the idea behind istrulyequal and ==? I am rather reluctant to allow use of == for non-mathematical equality... Could we perhaps instead use == for math equality and Julia's isequal for "cheap" comparison? Maybe not, depends on what your intention here were...

  • for groups, I think we also need an isfinite function (and perhaps also a hasisfinite / knowsisfinite / knows(:isfinite) / ... function)

  • for group elements, again, hasorder for me would indicate knowsorder and for you (I think) means "order is finite and can be computed... how about isfiniteorder or hasfiniteorder or so?

  • deepcopy is not yet implemented for GapObj from GAP.jl (see this issue), and implementing it in full generality is a bit tricky, but that doesn't mean it can't be implemented immediately for at least some cases (e.g. permutations); we should do this in GAP.jl ASAP, and then throw an error for cases which are not yet covered (which is infinitely better than what we do now, namely "crash"...)

  • order should specify what happens if it is invoked on an infinite group / element of infinite order

@kalmarek kalmarek linked a pull request Mar 13, 2021 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant