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.

Sign up for GitHub

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

Jump to bottom

Fix vector_space(K, polynomials) #3717

Merged
merged 2 commits into from
May 14, 2024
Merged

Fix vector_space(K, polynomials) #3717

merged 2 commits into from
May 14, 2024

Conversation

fingolfin
Copy link
Member

The preimage function this provided was buggy; it failed when the number of monomials involved exceeded the number of polynomials. The new tests catch this.

Also add a rudimentary doc string, and refactor the code to use exponent vectors instead of "monomials".

@Lax202 that should fix the case you had trouble with

@fingolfin
Fix vector_space(K, polynomials)
2503766 
The `preimage` function this provided was buggy; it failed when the number
of monomials involved exceeded the number of polynomials. The new tests
catch this.

Also add a rudimentary doc string, and refactor the code to use exponent
vectors instead of "monomials".
@fingolfin fingolfin requested a review from thofma May 11, 2024 11:30
@codecov Codecov
Copy link

codecov bot commented May 11, 2024

Codecov Report

Attention: Patch coverage is 97.05882% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 80.93%. Comparing base (5d26c4c) to head (2503766).

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #3717      +/-   ##
==========================================
- Coverage   81.34%   80.93%   -0.41%     
==========================================
  Files         577      577              
  Lines       78611    78580      -31     
==========================================
- Hits        63945    63598     -347     
- Misses      14666    14982     +316
Files Coverage Δ
src/Rings/mpoly-graded.jl 90.78% <97.05%> (-0.30%) ⬇️

... and 35 files with indirect coverage changes

joschmitt
joschmitt reviewed May 11, 2024
View reviewed changes
src/Rings/mpoly-graded.jl Outdated Show resolved Hide resolved
src/Rings/mpoly-graded.jl Outdated Show resolved Hide resolved
src/Rings/mpoly-graded.jl Show resolved Hide resolved
@fingolfin @joschmitt
Apply suggestions from code review
72002f5 
Co-authored-by: Johannes Schmitt <jschmitt@posteo.eu>
fingolfin
fingolfin commented May 13, 2024
View reviewed changes
src/Rings/mpoly-graded.jl
Comment on lines +1356 to +1357
Note that all polynomials must have the same parent `R`, and `K` must be equal
to `base_ring(R)`. The optional keyword argument `target` can be used to
Copy link
Member Author

Choose a reason for hiding this comment

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

Given this requirement on K, I wonder why we even have it as function argument? Perhaps we should at least make it optional?

Thoughts @thofma @fieker ?

Copy link
Member

Choose a reason for hiding this comment

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

This goes for some other vector_space methods as well:

vector_space(K::Field, e::Vector{T}; target) where T<:MPolyRingElem @ Oscar ~/.julia/dev/Oscar/src/Rings/mpoly-graded.jl:1349
vector_space(K::Field, Q::MPolyQuoRing) @ Oscar ~/.julia/dev/Oscar/src/Rings/MPolyQuo.jl:1272
vector_space(kk::Field, W::Oscar.MPolyQuoLocRing{<:Field, <:FieldElem, <:MPolyRing, <:MPolyRingElem, <:Oscar.MPolyComplementOfKPointIdeal}; ordering) @ Oscar ~/.julia/dev/Oscar/src/Rings/mpolyquo-localizations.jl:2203
vector_space(kk::Field, W::Oscar.MPolyQuoLocRing; ordering) @ Oscar ~/.julia/dev/Oscar/src/Rings/mpolyquo-localizations.jl:2184

Copy link
Collaborator

Choose a reason for hiding this comment

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

It construct a vector space and a map to the polynomial ring. If there are no polynomials, the polynomial ring (target of the map) must be supplied.

Copy link
Member Author

Choose a reason for hiding this comment

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

indeed, and that's why we have the target KW argument. But even in that scenario we don't need K -- after all we have this in the code: @assert base_ring(R) == K (where R is either target or else parent(polys[1]))

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah oops, I misunderstood, you are right.

joschmitt
joschmitt approved these changes May 13, 2024
View reviewed changes
Copy link
Member

@joschmitt joschmitt left a comment

Choose a reason for hiding this comment

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

The remaining question could also be resolved separately.

thofma
thofma reviewed May 13, 2024
View reviewed changes
Copy link
Collaborator

@thofma thofma left a comment

Choose a reason for hiding this comment

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

The code makes sense to me. Not sure if this accounts for much, since I never touched this method :). But if @joschmitt is happy, I am happy.

@fingolfin
Copy link
Member Author

@thofma ah I saw your name attached to the code in tig blame but that was a red herring from some global refactoring commits -- sorry! The true author of the code seems to be @fieker

@fingolfin fingolfin merged commit 93a0292 into oscar-system:master May 14, 2024
26 checks passed
@fingolfin fingolfin deleted the mh/fix-mpoly-vector_space branch May 14, 2024 14:59
@joschmitt joschmitt mentioned this pull request May 14, 2024
Merged
@joschmitt joschmitt added the backport 1.0.x Should be backported to the release 1.0 branch label May 24, 2024
@joschmitt
Copy link
Member

I added a backport label because this constitutes a bugfix. If this gets backported, then #3727 should also be backported.

benlorenz pushed a commit that referenced this pull request May 27, 2024
@fingolfin @benlorenz
Fix vector_space(K, polynomials) (#3717)
96daedd 
* Fix vector_space(K, polynomials)

The `preimage` function this provided was buggy; it failed when the number
of monomials involved exceeded the number of polynomials. The new tests
catch this.

Also add a rudimentary doc string, and refactor the code to use exponent
vectors instead of "monomials".

* Apply suggestions from code review

Co-authored-by: Johannes Schmitt <jschmitt@posteo.eu>

---------

Co-authored-by: Johannes Schmitt <jschmitt@posteo.eu>
(cherry picked from commit 93a0292)
@benlorenz benlorenz mentioned this pull request May 27, 2024
Merged
19 tasks
benlorenz added a commit that referenced this pull request May 27, 2024
@benlorenz
vector_space doctest: adjust to old printing style
622903c 
followup to the backport of #3717 and #3727
to adjust the printing to the old style on the release branch
@benlorenz benlorenz removed the backport 1.0.x Should be backported to the release 1.0 branch label May 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thofma thofma thofma left review comments

@joschmitt joschmitt joschmitt approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@fingolfin @joschmitt @thofma @benlorenz