-
Notifications
You must be signed in to change notification settings - Fork 14
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
doc: how to write a recognition method #250
Conversation
Codecov Report
@@ Coverage Diff @@
## master #250 +/- ##
=======================================
Coverage 78.50% 78.50%
=======================================
Files 37 37
Lines 17375 17376 +1
=======================================
+ Hits 13640 13641 +1
Misses 3735 3735
|
c32ab35
to
3fc063d
Compare
I've split up the content of our hack.md HOWTO: The rest of our hack.md HOWTO is in this PR. It adds a new chapter "How to write a recognition method". |
A leaf method must at the very least do the following: | ||
|
||
- Provide the order of the recognized group via `SetSize(ri, NNN)`. | ||
- Provide a set of SLPs which map the original generators $X$ to the nice generators $Y$, as entry for the attribute `slptonice`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe for consistency, say that this can be set via Setslptonice(ri, slp)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the reader does not know how to set an attribute in GAP, I think they need to read some other tutorial first :-).
However, we could point out that all of these are show in the example later on, and maybe also add a Ref to that example
|
||
- Provide the order of the recognized group via `SetSize(ri, NNN)`. | ||
- Provide a set of SLPs which map the original generators $X$ to the nice generators $Y$, as entry for the attribute `slptonice`. | ||
- Provide a function which maps any element $g\in G$ to a corresponding SLP in terms of the nice generators $Y$, as entry for the attribute `slpforelement`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, one could mention Setslpforelement(ri, func)
.
The input is in the format described above (TODO), and the return value is "Success". | ||
|
||
Two more comments: | ||
- When we check whether all generators are the identity, we call `ri!.isone`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also use isone(ri)
. I do not know which version is preferred.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both are used in recog code right now.
Formally, since isone
is an attribute, the "official" way to access it is isone(ri)
, while using ri!.isone
relies on an implementation detail. HOWEVER, I personally find ri!.isone(x)
much clearer than isone(ri)(x)
, and even more so for ri!.isequal(x,y)
vers. isequal(ri)(x,y)
.
That said, it doesn't really matter much... My hope is that on the long run, we can get rid of these again, by using "proper" projective matrices (see #82); and before that, perhaps they get renamed (see #97).
Quick Question: Do we want to say anything about verification in this chapter? For example certain leaf methods verify that the constructive recognition was correct. |
Ah nice, yes. Do you know an example that does this @FriedrichRober ? |
No I do not know of an example that does this. I am not sure, how verification is handled in recog. Maybe @fingolfin knows more about this? |
Finding out how verification works actually still is on my todo list. I have no idea how it works. :D |
This HOWTO has sat unreviewed and unused for years now; I recommend we merge it and then fret over improving it, instead of getting hold up in minor details... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, as Max said, I think we should get this merged. Improvements can come in separate PRs. (The typo comment doesn't need to be addressed now).
Co-authored-by: Wilf Wilson <[email protected]>
I've added TODOs based on your suggestions and fixed the typos @wilfwilson found. If the tests pass, I'll merge this in. |
Fixes #148.