[REVIEW]: hilbertmodgroup: Reduction algorithms and framework for Hilbert Modular Groups

whedon commented 2 years ago

Submitting author: !--author-handle-->@fredstro@danielskatz<!--end-editor-- Reviewers: @tbirkandan, @videlec Archive: 10.5281/zenodo.6422510

:warning: JOSS reduced service mode :warning:

Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post.

Status

Status badge code:

HTML: <a href="https://joss.theoj.org/papers/a75a04fc8fcfc7c5af394c7ab62a8828"><img src="https://joss.theoj.org/papers/a75a04fc8fcfc7c5af394c7ab62a8828/status.svg"></a>
Markdown: [![status](https://joss.theoj.org/papers/a75a04fc8fcfc7c5af394c7ab62a8828/status.svg)](https://joss.theoj.org/papers/a75a04fc8fcfc7c5af394c7ab62a8828)

Reviewers and authors:

Please avoid lengthy details of difficulties in the review thread. Instead, please create a new issue in the target repository and link to those issues (especially acceptance-blockers) by leaving comments in the review thread below. (For completists: if the target issue tracker is also on GitHub, linking the review thread in the issue or vice versa will create corresponding breadcrumb trails in the link target.)

Reviewer instructions & questions

@tbirkandan & @videlec, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:

Make sure you're logged in to your GitHub account
Be sure to accept the invite at this URL: https://github.com/openjournals/joss-reviews/invitations

The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @danielskatz know.

✨ Please start on your review when you are able, and be sure to complete your review in the next six weeks, at the very latest ✨

Review checklist for @tbirkandan

✨ Important: Please do not use the Convert to issue functionality when working through this checklist, instead, please open any new issues associated with your review in the software repository associated with the submission. ✨

Conflict of interest

[x] I confirm that I have read the JOSS conflict of interest (COI) policy and that: I have no COIs with reviewing this work or that any perceived COIs have been waived by JOSS for the purpose of this review.

Code of Conduct

[x] I confirm that I read and will adhere to the JOSS code of conduct.

General checks

[x] Repository: Is the source code for this software available at the repository url?
[x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
[x] Contribution and authorship: Has the submitting author (@fredstro) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?
[x] Substantial scholarly effort: Does this submission meet the scope eligibility described in the JOSS guidelines

Functionality

[x] Installation: Does installation proceed as outlined in the documentation?
[x] Functionality: Have the functional claims of the software been confirmed?
[x] Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
[x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
[x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
[x] Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
[x] Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

[x] Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
[x] A statement of need: Does the paper have a section titled 'Statement of Need' that clearly states what problems the software is designed to solve and who the target audience is?
[x] State of the field: Do the authors describe how this software compares to other commonly-used packages?
[x] Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
[x] References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax?

Review checklist for @videlec

✨ Important: Please do not use the Convert to issue functionality when working through this checklist, instead, please open any new issues associated with your review in the software repository associated with the submission. ✨

Conflict of interest

[x] I confirm that I have read the JOSS conflict of interest (COI) policy and that: I have no COIs with reviewing this work or that any perceived COIs have been waived by JOSS for the purpose of this review.

Code of Conduct

[x] I confirm that I read and will adhere to the JOSS code of conduct.

General checks

[x] Repository: Is the source code for this software available at the repository url?
[x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
[x] Contribution and authorship: Has the submitting author (@fredstro) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?
[x] Substantial scholarly effort: Does this submission meet the scope eligibility described in the JOSS guidelines

Functionality

[x] Installation: Does installation proceed as outlined in the documentation?
[x] Functionality: Have the functional claims of the software been confirmed?
[x] Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
[x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
[x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
[x] Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
[x] Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

[x] Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
[x] A statement of need: Does the paper have a section titled 'Statement of Need' that clearly states what problems the software is designed to solve and who the target audience is?
[x] State of the field: Do the authors describe how this software compares to other commonly-used packages?
[x] Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
[x] References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax?

danielskatz commented 2 years ago

@videlec - I wonder if you can make progress on other parts of the review, such as the paper and documentation?

danielskatz commented 2 years ago

👋 @fredstro & @videlec - another ping on the questions immediately above...

fredstro commented 2 years ago

I have addressed all issues except https://github.com/fredstro/hilbertmodgroup/issues/5 for which I don't really know what the best solution is. It is a really complex question about Python packaging and pip build isolation. I don't know if @videlec agrees but for the purpose of the publication in JOSS I think that any of my proposed solutions should be sufficient and the search for an optimal solution if there even is one could be left for later.

videlec commented 2 years ago

I have addressed all issues except fredstro/hilbertmodgroup#5 for which I don't really know what the best solution is. It is a really complex question about Python packaging and pip build isolation. I don't know if @videlec agrees but for the purpose of the publication in JOSS I think that any of my proposed solutions should be sufficient and the search for an optimal solution if there even is one could be left for later.

I do agree : the hilbertmodgroup package does not have to solve the packaging issues that SageMath is facing! Especially in the process of the JOSS review.

danielskatz commented 2 years ago

thanks @videlec - when do you think you might complete your review?

fredstro commented 2 years ago

The associated research paper which was referenced has been accepted by the Journal of Number Theory so I have updated the references. I also updated the referenced version of Sage to 9.5. I will tell whedon to update the paper.

fredstro commented 2 years ago

@whedon generate pdf from branch paper_joss

editorialbot commented 2 years ago

My name is now @editorialbot

videlec commented 2 years ago

@fredstro (not really part of the review) : from your code is it possible to get a presentation of SL(2, O_K) (that is generators and relations)?

videlec commented 2 years ago

@fredstro : You implemented a reduction algorithm but I was not able to spot the actual reduction function from (H^2)^degree -> (H^2)^degree. I expected to see something like

sage: p = a random point in (H^2)^degree
sage: q = (some matrix in SL2OK) * p
sage: SL2OK.reduce(p) == SL2OK.reduce(q)
True

Such reduce function could be enriched with the matrix getting to the fundamental domain.

videlec commented 2 years ago

(sorry for my very naive questions)

fredstro commented 2 years ago

It is trivial to write down a set of generators but it is very hard to find a set of "side-pairing" generators and get out a presentation from the geometry (which is the most conceptual way of doing it over Q). The code can be used to do this numerically but it is not easy as the "bottom" of the fundamental domain is very complicated in general. I am working on another algorithm (which will use parts of this one) to find a reduced set of elliptic fixed-points and associated elements, which will result in a presentation.

For the moment I tried to make the Hilbert modular group classes having the same structure as the subgroups of the modular group and I didn't want to clutter it too much, which is why I introduced the utility class "HilbertPullback" which contains all functionality regarding the reduction. So at the moment it would be: sage: p = a random point in (H^2)^degree sage: q = (some matrix in SL2OK) * p sage: R = HilbertPullback(SL2OK) sage: R.reduce(p) == R.reduce(q) True

I was thinking of having a "reduce" function also in the HilbertModularGroup but then I would get a circular dependency so I am not sure what the best way is to do that.

videlec commented 2 years ago

Using HilbertPullback is perfectly fine (this reduction is not attached to the group anyway, you did a choice of fundamental domain). Could you had this P1.reduce early in the ExamplesK1.ipynb?

fredstro commented 2 years ago

Yes, I can move it earlier up.

danielskatz commented 2 years ago

@editorialbot set paper_joss as branch

editorialbot commented 2 years ago

Done! branch is now paper_joss

danielskatz commented 2 years ago

@editorialbot check references

editorialbot commented 2 years ago

Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1109/mcse.2010.118 is OK
- 10.1016/j.jnt.2022.02.011 is OK
- 10.1007/s10711-019-00474-w is OK
- 10.1112/S1461157015000121 is OK

MISSING DOIs

- None

INVALID DOIs

- None

danielskatz commented 2 years ago

@editorialbot generate pdf

editorialbot commented 2 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

danielskatz commented 2 years ago

@whedon generate pdf from branch paper_joss

@fredstro - as you can see above, editorialbot is the new whedon, and once the branch is set (as it is above), you just need to use @editorialbot generate pdf to make a new pdf

danielskatz commented 2 years ago

👋 @videlec - can you again update us on what you need to make progress on your review?

videlec commented 2 years ago

@fredstro Concerning "Community guidelines" it would be good to mention in the README 1) how people could contribute to the software? (I guess opening issues and opening merge requests?) 2) Report issues or problems with the software? (I guess opening issues) 3) Seek support?

videlec commented 2 years ago

@danielskatz The above comment together with https://github.com/fredstro/hilbertmodgroup/issues/9 are my last comments for the review. The hilbertmodgroup library is in very good shape.

fredstro commented 2 years ago

I have addressed issue #9 and I have also updated the README file with community guidelines (all in "develop")

videlec commented 2 years ago

What is the point of having a develop and a main branch (the latter being the default)? The tags contains the relevant pointers to the commit corresponding to specific versions.

fredstro commented 2 years ago

The point is to have a develop branch for development and bugfixes and once the development is verified as "correct" it will be merged into main. I am only using a single "develop" branch since I am the only developer but if more people gets involved I will use git-flow with feature branches.

Even with tags and pointers to specific versions there will be users that just clone the repo and try to instal the latest main branch so it is good to keep that as clean and working as possible in each commit. I am probably treating "main" more as a "release" branch...

If you think this is a wrong workflow and have a better suggestion just let me know.

videlec commented 2 years ago

It is not wrong but it ought to be documented.

fredstro commented 2 years ago

Ok

fredstro commented 2 years ago

I have added documentation about use of branches. Let me know if it is not clear enough.

danielskatz commented 2 years ago

Thanks for the recent activity on this, @videlec & @fredstro.

@videlec, can you confirm that the latest changes are sufficient and you are happy for this to be published?

videlec commented 2 years ago

@danielskatz I do confirm that this is in very good shape for publication.

danielskatz commented 2 years ago

@fredstro - at this point, I'm going to proofread the paper, then come back to you with the next steps.

danielskatz commented 2 years ago

@editorialbot generate pdf

danielskatz commented 2 years ago

@editorialbot check references

editorialbot commented 2 years ago

Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1109/mcse.2010.118 is OK
- 10.1016/j.jnt.2022.02.011 is OK
- 10.1007/s10711-019-00474-w is OK
- 10.1112/S1461157015000121 is OK

MISSING DOIs

- None

INVALID DOIs

- None

editorialbot commented 2 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

danielskatz commented 2 years ago

@fredstro - I'm suggesting some small changes in https://github.com/fredstro/hilbertmodgroup/pull/10. In addition, I think that perhaps you should formally cite Guass' work in the third paragraph, rather than just mentioning it inline.

fredstro commented 2 years ago

Ok. I have merged your suggestions and added a formal reference to (the English translation) of Gauss' work.

fredstro commented 2 years ago

@editorialbot generate pdf

editorialbot commented 2 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

fredstro commented 2 years ago

@editorialbot check references

editorialbot commented 2 years ago

Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1109/mcse.2010.118 is OK
- 10.1016/j.jnt.2022.02.011 is OK
- 10.1007/s10711-019-00474-w is OK
- 10.1112/S1461157015000121 is OK

MISSING DOIs

- None

INVALID DOIs

- None

fredstro commented 2 years ago

Sorry, the commit containing this was not pushed....

fredstro commented 2 years ago

@editorialbot generate pdf

fredstro commented 2 years ago

@editorialbot check references

editorialbot commented 2 years ago

Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1109/mcse.2010.118 is OK
- 10.1016/j.jnt.2022.02.011 is OK
- 10.1007/s10711-019-00474-w is OK
- 10.1112/S1461157015000121 is OK

MISSING DOIs

- None

INVALID DOIs

- None

editorialbot commented 2 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

danielskatz commented 2 years ago

@fredstro - At this point could you:

[ ] Make a tagged release of your software, and list the version tag of the archived version here.
[ ] Archive the reviewed software in Zenodo or a similar service (e.g., figshare, an institutional repository)
[ ] Check the archival deposit (e.g., in Zenodo) has the correct metadata. This includes the title (should match the paper title) and author list (make sure the list is correct and people who only made a small fix are not on it). You may also add the authors' ORCID.
[ ] Please list the DOI of the archived version here.

I can then move forward with accepting the submission.

fredstro commented 2 years ago

I have created at tagged 0.1.0 release and uploaded it to PyPi as well as archived it in Zenodo. In Zenodo I uploaded both the source distribution and a zip file of the complete git repository at this tag from GitHub.
I have checked that the metadata is correct (and I included my ORCID).

The DOI is: 10.5281/zenodo.6422510

openjournals / joss-reviews