construct AdditiveAbelianGroupWrapper from (not necessarily independent) generating set

[yyyyx4]

Currently, constructing an AdditiveAbelianGroupWrapper requires prior knowledge of an independent set of generators for the group. This patch adds an algorithm (implemented following Sutherland's thesis) to construct an AdditiveAbelianGroupWrapper from an arbitrary generating set, whose members need not be independent. This is useful for all kinds of things, including structure computations for finite abelian groups.

[codecov-commenter]

Codecov Report

Patch coverage: 70.47% and project coverage change: -0.01 ⚠️

Comparison is base (f449b14) 88.62% compared to head (33f5f73) 88.61%.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #35270      +/-   ##
===========================================
- Coverage    88.62%   88.61%   -0.01%     
===========================================
  Files         2148     2148              
  Lines       398653   398754     +101     
===========================================
+ Hits        353308   353369      +61     
- Misses       45345    45385      +40     

Impacted Files	Coverage Δ
...c/sage/schemes/elliptic_curves/ell_finite_field.py	88.96% <ø> (ø)
...roups/additive_abelian/additive_abelian_wrapper.py	86.49% <70.47%> (-12.77%)	⬇️

... and 25 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[tscrim]

I would remove the commented out lines.

Also _expand_basis_pgroup doesn't seem to be called from anywhere within the library. Should it be? Or have I missed something?

[yyyyx4]

I would remove the commented out lines.

They are commented out for performance, but they will be very helpful to anyone who might later want to understand or debug or modify this code. I can replace them with textual descriptions of the expected behaviour of the code if you prefer, but to me the assertions seem better. What I would actually like is for Sage to disable assertions (python -O) in production so that we can simply leave them in without worrying about performance.

Also _expand_basis_pgroup doesn't seem to be called from anywhere within the library. Should it be? Or have I missed something?

It is called here.

[tscrim]

I would remove the commented out lines.

They are commented out for performance, but they will be very helpful to anyone who might later want to understand or debug or modify this code. I can replace them with textual descriptions of the expected behaviour of the code if you prefer, but to me the assertions seem better. What I would actually like is for Sage to disable assertions (python -O) in production so that we can simply leave them in without worrying about performance.

True; I full agree with you about being able to disable assert statements. Well, then we can leave them in there, at least until someone “cleaning” the code goes through and removes them. Perhaps a comment or two about them to help make sure they don’t get removed accidentally.

Personal preference, I like the comment # to begin at the same level as the code. It makes it easier to read when I am not using an editor that colors the code and this is the general style within Sage. Although I am not making it a requirement of a positive review.

Also _expand_basis_pgroup doesn't seem to be called from anywhere within the library. Should it be? Or have I missed something?

It is called here.

Whoops, yes indeed.

[github-actions]

Documentation preview for this PR is ready! 🎉
Built with commit: 33f5f73

[tscrim]

Thank you. LGTM.

[yyyyx4]

Great, thanks a lot! 🙂