Improve documentation of parts of chapter 41 in the reference manual #3591
Conversation
PaulaHaehndel
added
release notes: to be added
|
Does this resolve any open issues? If so, please could you edit your first post in this PR (I.e. the description) to automatically close those issues (see https://help.github.com/en/articles/closing-issues-using-keywords) |
|
As far as I'm aware there are no open issues that will be closed by this PR. |
|
This is essentially the same comment for the changes with your formulation, so I did not make them everywhere. If you want, you can change this, but I don't insist on it. |
|
I would suggest that you squash you commits into one commit when you're done and change the commit message. |
Codecov Report
@@ Coverage Diff @@
## master #3591 +/- ##
==========================================
+ Coverage 77.24% 84.54% +7.3%
==========================================
Files 692 697 +5
Lines 339572 344995 +5423
==========================================
+ Hits 262306 291690 +29384
+ Misses 77266 53305 -23961
|
PaulaHaehndel
force-pushed
the
docchap41
branch
from
2c782b9
to
24a6a19
Compare
There was a problem hiding this comment.
In my dream universe, this whole chapter of documentation would be significantly rewritten, or possibly rewritten from scratch. However, that's not a good reason to discourage improvements to the currently existing version! I think that this PR does provide useful improvements, and it doesn't make things worse, so I'm happy for it to be merged.
I had one English grammar change suggestion.
Finally, one more topic... perhaps way too much detail...
I wanted to make a comment similar to the one made by @DominikBernhardt about how a permutation group acts on a set of points rather than points.
I see what you mean about a possible inconsistency. But for me, it's not so bad, and the changes that you made to turn it into a set do make things better. When I read something like:
...natural action on the points moved by it
I automatically and subconsciously add the words "set of" to make it:
...action on the set of points moved by it
It feels like a natural way of talking about these things, in English at least. It's just being a tiny bit lazy and not being ultra precise, but I feel like it's perfectly clear. Whereas for me, saying something like
...the group acts on the points 2, 3, 4, 5, 6.
feels very detailed, and so the way it sounds is measured against a different standard. And it just doesn't sound right. (I don't remember what you wrote exactly). Therefore I prefer the newer version.
If the inconsistency bugs you, I suggest you add in "set of" to all of those previous sentences.
lib/oprt.gd
Outdated
| ## <Attr Name="OrbitLengths" Arg='xset' Label="for an external set"/> | ||
| ## | ||
| ## <Description> | ||
| ## computes the lengths of all the orbits of the elements in <A>seeds</A> | ||
| ## under the action <A>act</A> of <A>G</A>. | ||
| ## <P/> | ||
| ## For a permutation group <A>G</A>, one may also invoke this as | ||
| ## <C>OrbitLengths(<A>G</A>)</C>, which returns the length of all the |
There was a problem hiding this comment.
| ## <C>OrbitLengths(<A>G</A>)</C>, which returns the length of all the | |
| ## <C>OrbitLengths(<A>G</A>)</C>, which returns the lengths of all the |
|
As @wilfwilson said, this chapter should probably be rewritten, but you changes are a step in the right direction, so thank you for the improvement! I agree with the changes. One more thing @PaulaHaehndel and @wilfwilson : This PR now consists of 6 commits. Maybe you could squash it into one and write a new commit message? I would then be happy with having this merged. |
|
Whoever merges it could instead do a “Squash and merge” which would squash them into one commit automatically while merging. I don’t think it matters so much what happens - I’ll leave it up to @PaulaHaehndel |
DominikBernhardt
added
release notes: added
I tried to update some (not all) of the functions in the context of group actions (documented in chapter 41 in the reference manual) such that the documentation matches the behaviour of GAP.
While doing so I included some more examples.