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

Do not cache polynomial rings #3838

Merged
merged 7 commits into from
Jun 17, 2024
Merged

Conversation

ederc
Copy link
Member

@ederc ederc commented Jun 10, 2024

This PR disables caching of polynomial_ring in Rings/*. See discussion in #2455.

@ederc ederc marked this pull request as draft June 10, 2024 18:53
@ederc
Copy link
Member Author

ederc commented Jun 10, 2024

@HechtiDerLachs Any idea what to do with these tests from test/Rings/ReesAlgebra.jl?

  # Both algebras should be the same despite being created differently.
  @test all(x->iszero(RM(x)), gens(modulus(Rg)))
  @test all(x->iszero(Rg(x)), gens(modulus(RM)))

@HechtiDerLachs
Copy link
Collaborator

I'm afraid, I do not yet fully understand the question, sorry. Should I run the test on this branch and look into the error messages?

@ederc
Copy link
Member Author

ederc commented Jun 10, 2024

These tests will fail since the corresponding requirement that the parents are equal no longer holds. As discussed in #2455 this is the behaviour wanted. I just do not know how to adjust the corresponding tests, this is something I would like you to decide.

@HechtiDerLachs
Copy link
Collaborator

Alright, thanks for the clarification. At the moment I can not so easily switch branches since I'm currently working on something else, but I will look into it the next possible moment.

@fingolfin
Copy link
Member

Orthogonal to this issue, but: @HechtiDerLachs you can have multiple Oscar.jl clones in parallel (or even use a single clone with multiple worktrees). So that you can e.g. have Oscar master and your active development branch and another branch all ready and running in different Julia sessions... I can happily help you set that up, too (not saying this to put pressure on you to look into this issue, just to improve your workflow in the future)

@HechtiDerLachs
Copy link
Collaborator

@ederc : Could you replace the lines in the test by the following:

@test all(x->iszero(evaluate(x, gens(RM))), gens(modulus(Rg)))
@test all(x->iszero(evaluate(x, gens(Rg))), gens(modulus(RM)))

I think that should do the job. At least it does for me locally.
Thank you and sorry about the delay.

@ederc ederc marked this pull request as ready for review June 15, 2024 05:18
@ederc
Copy link
Member Author

ederc commented Jun 15, 2024

@HechtiDerLachs Thanks, I applied your changes.

Copy link

codecov bot commented Jun 15, 2024

Codecov Report

Attention: Patch coverage is 85.45455% with 8 lines in your changes missing coverage. Please review.

Project coverage is 81.83%. Comparing base (5e624ff) to head (608c6f1).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3838      +/-   ##
==========================================
- Coverage   81.83%   81.83%   -0.01%     
==========================================
  Files         581      581              
  Lines       80002    80002              
==========================================
- Hits        65468    65466       -2     
- Misses      14534    14536       +2     
Files Coverage Δ
src/Rings/AbelianClosure.jl 94.20% <100.00%> (ø)
src/Rings/Laurent.jl 87.27% <100.00%> (ø)
src/Rings/MPolyMap/AffineAlgebras.jl 96.00% <100.00%> (ø)
src/Rings/MPolyMap/MPolyAnyMap.jl 92.85% <100.00%> (ø)
src/Rings/MPolyMap/flattenings.jl 92.19% <100.00%> (ø)
src/Rings/NumberField.jl 90.22% <100.00%> (ø)
src/Rings/PBWAlgebra.jl 94.89% <100.00%> (ø)
src/Rings/ReesAlgebra.jl 90.90% <100.00%> (ø)
src/Rings/groebner.jl 90.04% <100.00%> (ø)
src/Rings/mpoly-affine-algebras.jl 82.36% <100.00%> (ø)
... and 10 more

... and 1 file with indirect coverage 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.

Thank you!

@joschmitt joschmitt merged commit b1cc9b7 into oscar-system:master Jun 17, 2024
28 of 29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants