Skip to content

Conversation

@fingolfin
Copy link
Member

@fingolfin fingolfin commented Dec 9, 2025

Also reformat some code.

Extracted from PR #2108

@fingolfin fingolfin changed the title Add missing deepcopy for Generic.Ideal Add missing deepcopy for Generic.Ideal Dec 9, 2025
Also reformat some code.
@fingolfin fingolfin force-pushed the mh/deepcopy-Generic.Ideal branch from 57efa29 to 2b778fe Compare December 9, 2025 23:41
@codecov
Copy link

codecov bot commented Dec 9, 2025

Codecov Report

❌ Patch coverage is 96.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 88.00%. Comparing base (2627b5e) to head (d0825c5).
⚠️ Report is 4 commits behind head on master.

Files with missing lines Patch % Lines
src/generic/SparsePoly.jl 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2236   +/-   ##
=======================================
  Coverage   88.00%   88.00%           
=======================================
  Files         127      127           
  Lines       31798    31809   +11     
=======================================
+ Hits        27983    27995   +12     
+ Misses       3815     3814    -1     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Collaborator

@SoongNoonien SoongNoonien left a comment

Choose a reason for hiding this comment

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

The changes seem fine. But I think we should add an explicit test which verifies that deepcopy does what it should when intersecting ideals.

@fingolfin fingolfin force-pushed the mh/deepcopy-Generic.Ideal branch from ce5c123 to d0825c5 Compare December 10, 2025 09:39
@fingolfin fingolfin changed the title Add missing deepcopy for Generic.Ideal Generic.Ideal: add missing deepcopy; add and use check_base_ring helper Dec 10, 2025
@fingolfin
Copy link
Member Author

I've added the check_base_ring from PR #2108 and inserted it into various Generic.Ideal method. That triggers the new deepcopy method -- and without it, the tests would fail:

Generic.Ideal.adhoc_multiplication |  396    396  0.4s
Generic.Ideal.intersect: Error During Test at /Users/mhorn/Projekte/OSCAR/AbstractAlgebra.jl/test/generic/Ideal-test.jl:667
  Test threw exception
  Expression: is_subset(K, I)
  base rings do not match
  Stacktrace:
   [1] error(s::String)
     @ Base ./error.jl:44
   [2] check_base_ring
     @ .../AbstractAlgebra.jl/src/AbstractAlgebra.jl:238 [inlined]
   [3] check_base_ring
     @ .../AbstractAlgebra.jl/src/AbstractAlgebra.jl:237 [inlined]
   [4] issubset(I::AbstractAlgebra.Generic.Ideal{AbstractAlgebra.Generic.Poly{AbstractAlgebra.GFElem{Int64}}}, J::AbstractAlgebra.Generic.Ideal{AbstractAlgebra.Generic.Poly{AbstractAlgebra.GFElem{Int64}}})
     @ AbstractAlgebra.Generic .../AbstractAlgebra.jl/src/generic/Ideal.jl:2122
...

@fingolfin fingolfin enabled auto-merge (squash) December 10, 2025 09:47
@fingolfin fingolfin merged commit 811e27c into master Dec 10, 2025
23 checks passed
@fingolfin fingolfin deleted the mh/deepcopy-Generic.Ideal branch December 10, 2025 13:43
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.

2 participants