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

Improve doc of gap/matrix/slconstr.gi #252

Merged
merged 1 commit into from
Apr 20, 2021

Conversation

ssiccha
Copy link
Collaborator

@ssiccha ssiccha commented Apr 2, 2021

  • add a header comment which of these functions is actually used
  • document SLCR.FindHom
  • reference SLCR.FindHom in the documentation of
    FindHomMethodsMatrix.NaturalSL

TODO: the documentation of SLCR.FindHom does not appear in the manual.
@fingolfin, could you point me to the file I need to change? I had a look but didn't find anything.

@ssiccha
Copy link
Collaborator Author

ssiccha commented Apr 2, 2021

@danielrademacher Maybe this is also interesting for you.

@ssiccha
Copy link
Collaborator Author

ssiccha commented Apr 2, 2021

Also, FindHom should be renamed. While we're at it, what would be a good naming convention for leaf methods so that they can be easily grepped for? Maybe that's a topic for our next call.

@codecov
Copy link

codecov bot commented Apr 2, 2021

Codecov Report

Merging #252 (f49b3ff) into master (f4cf7d3) will not change coverage.
The diff coverage is n/a.

❗ Current head f49b3ff differs from pull request most recent head 208e0e5. Consider uploading reports for the commit 208e0e5 to get more accurate results

@@           Coverage Diff           @@
##           master     #252   +/-   ##
=======================================
  Coverage   77.76%   77.76%           
=======================================
  Files          37       37           
  Lines       17497    17497           
=======================================
  Hits        13606    13606           
  Misses       3891     3891           
Impacted Files Coverage Δ
gap/base/methsel.gd 100.00% <ø> (ø)
gap/matrix/slconstr.gi 5.88% <ø> (ø)

@danielrademacher
Copy link
Collaborator

Yes, that's great to know! Thanks!
I have to check what function is used in Akos and Max code and to run some tests which functions are currently best in GAP for constructive recognition.

And I agree with Sergio, we should rename the function and choose clearer names to differentiate which function does what. The names should be chosen consistently in a certain way. Then it would be easier for users and programmers of the package. But that is probably connected with an enormous amount of effort. Shall we open an issue about this?

@ssiccha
Copy link
Collaborator Author

ssiccha commented Apr 2, 2021

@danielrademacher To in general come up with better names for many functions we already have an issue, see #177 and our note with proposals for new names which we made during the summer school. For #177 @FriedrichRober and I only looked into the documented names though. Thanks for the comment, I'm looking into finding a few names from our proposal note to open a PR with. That has also been lying around for way too long.

Back to the FindHom function. That one was not documented before. So we didn't consider it for #177 yet. Also we had not thought about a naming scheme for leaf methods yet. I guess in the documentation we should also separate the splitting methods and the leaf methods.

@ssiccha
Copy link
Collaborator Author

ssiccha commented Apr 2, 2021

@danielrademacher See #253.

gap/matrix/slconstr.gi Outdated Show resolved Hide resolved
@@ -2465,7 +2471,7 @@ SLPforElementFuncsMatrix.SLConstructive := function(ri,x)
end;

#! @BeginChunk NaturalSL
#! TODO
#! TODO: This function is not used at the moment, but SLCR.FindHom is used.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Iissue #257 would also take care of this comment

Copy link
Collaborator Author

@ssiccha ssiccha Apr 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm removing this change and will do #257 and #258 next.

- add a header comment which of these functions is actually used
- document SLCR.FindHom

Co-authored-by: Max Horn <[email protected]>
@ssiccha
Copy link
Collaborator Author

ssiccha commented Apr 7, 2021

Added suggestions and rebased.

Copy link
Collaborator

@danielrademacher danielrademacher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

@fingolfin fingolfin merged commit 37f81b4 into gap-packages:master Apr 20, 2021
@ssiccha ssiccha deleted the ss/doc-slconstr branch April 21, 2021 08:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants