Fix edge-case in ConformanceTests.generate_element(::MPolyRing)#2077
Merged
lgoettgens merged 3 commits intoNemocas:masterfrom May 23, 2025
Merged
Fix edge-case in ConformanceTests.generate_element(::MPolyRing)#2077lgoettgens merged 3 commits intoNemocas:masterfrom
ConformanceTests.generate_element(::MPolyRing)#2077lgoettgens merged 3 commits intoNemocas:masterfrom
Conversation
HechtiDerLachs
commented
May 14, 2025
| exp_bound = rand(1:5) | ||
| len = rand(0:len_bound) | ||
| coeffs = [ConformanceTests.generate_element(R) for _ in 1:len] | ||
| coeffs = elem_type(R)[ConformanceTests.generate_element(R) for _ in 1:len] |
Contributor
Author
There was a problem hiding this comment.
In case of empty lists, this was of type Vector{Any} despite the generation function being type stable. This caused more problems down the road.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2077 +/- ##
==========================================
- Coverage 88.39% 88.37% -0.02%
==========================================
Files 125 126 +1
Lines 31578 31639 +61
==========================================
+ Hits 27914 27962 +48
- Misses 3664 3677 +13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
Author
|
Any volunteers for approval? @thofma ? @lgoettgens ? |
Member
|
Hm, maybe @fingolfin and @lgoettgens can comment? They worked on the interface tests and probably know if this was intended. |
fingolfin
reviewed
May 15, 2025
| function should only be called with `check=false` if it is already known that the | ||
| division will be exact. | ||
|
|
||
| ***Note:*** For the *recursive* ring tests to work, one needs to also implement `divides(f::MyElem, g::MyElem)`. |
Member
There was a problem hiding this comment.
I think divides is only required for euclidean rings: only test_EuclideanRing_interface calls it. Which is in turn only invoked automatically for univariate polynomial rings over a field...
Member
There was a problem hiding this comment.
And there are generic divides methods for that, i.e.:
divides(f::PolyRingElem{T}, g::PolyRingElem{T}) where T <: RingElement
divides(z::PolyRingElem{T}, x::T) where T <: RingElement
So it would be really good to know what the actual problem you run into was?
Member
|
This was the backtrace leading to the problem (taking from Slack): |
Member
|
So this started with As a recursive test was requested it then went to test mpoly rings over the given monoid algebra, i.e.: which then calls And that test then ends up doing: This eventually triggers a call to and so on. So how to handle this. Some option:
For 3 one possible way forward would be via the |
Member
|
All that said, I really don't want |
Member
|
Not clear what the "ring interface" promises. "If you implement these methods for your ring type, then the recursive tests will pass (modulo bugs in your code)"? I am happy to try out 3). If this is not an option (because of time reasons), what about hiding the test behind a |
This reverts commit 7f6d41f.
HechtiDerLachs
changed the title
lgoettgens
changed the title
ConformanceTests.generate_element(::MPolyRing)
lgoettgens
approved these changes
May 23, 2025
Contributor
Author
|
I updated this in view of #2096 . Now it does not contain changes to the docu anymore. |
lgoettgens
changed the title
ConformanceTests.generate_element(::MPolyRing)ConformanceTests.generate_element(::MPolyRing)
fingolfin
approved these changes
May 23, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
It turns out that for the recursive ring interface tests one actually needs to implement
divides. We should mention that in the documentation of the ring interface.