Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

add number_of_generators for UniversalPolyRing #1795

Merged
merged 1 commit into from
Sep 20, 2024

Conversation

SoongNoonien
Copy link
Contributor

It seems like the poor universal polynomials were overlooked again ;-) I tried to make this somehow in line with the MPoly implementation. In the documentation examples for the MPolys we are testing number_of_generators for equality instead of simply printing the result like in the new examples for UniversalPolyRing. Is there a particular reason for this?

@lgoettgens
Copy link
Collaborator

Thanks for your PR. At first glance, this looks good. But since we don't have the compatibility tests with Hecke and Oscar running at the moment (AbstractAlgebra already jumped to the next minor version, but downstream hasn't been adapted), I would like to wait (a few days) until this is working again, just to make sure to not accidentally break something.

Copy link

codecov bot commented Sep 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.15%. Comparing base (ed58b14) to head (44a38d1).
Report is 11 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1795      +/-   ##
==========================================
+ Coverage   88.01%   88.15%   +0.14%     
==========================================
  Files         118      119       +1     
  Lines       30043    31271    +1228     
==========================================
+ Hits        26442    27567    +1125     
- Misses       3601     3704     +103     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lgoettgens lgoettgens closed this Sep 20, 2024
@lgoettgens lgoettgens reopened this Sep 20, 2024
@fingolfin fingolfin closed this Sep 20, 2024
@fingolfin fingolfin reopened this Sep 20, 2024
Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

I find it a bit weird to ask for the number of generators of an universal polynomial ring given that it is can change at any time. But if one takes it as an enhanced version of ngens(R) = length(gens(R)) then the code in here of course is fine. Also we already do this for number_of_variables, so...

@fingolfin fingolfin merged commit fd3af98 into Nemocas:master Sep 20, 2024
68 of 70 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants