Add conformance tests for VarietyFunctionField, add base_ring_type method
#4417
Conversation
Also add missing base_ring_type method.
It seems to be only used for one thing, coercing Integer values into VarietyFunctionField elements
|
Unfortunately this requires Nemocas/AbstractAlgebra.jl#1946 which is not yet in an AA release. So turning this into a draft for now. But the general gist here is: all our custom ring types should be run through our conformance tests. |
There was a problem hiding this comment.
Thanks for cleaning this up. I would keep base_ring and introduce base_ring_type. It's just the first type parameter of VarietyFunctionField.
As for the test_elem: I think we can copy from what we have for polynomial rings and use that for numerator and denominator?
If you agree, I can check out this PR and do the changes (once I manage to fix up my github client, that is...).
No, it is not: and I ended up with base_ring_type(::Type{T}) where {FracFieldType, T<:VarietyFunctionField{<:Field, FracFieldType}} = base_ring_type(FracFieldType)to match what
Perhaps, I have no idea how to even construct any element of
Sure!
You can always make another git worktree (or two, or three... ;-) ) (But to be clear, this is not urgent in anyway) |
|
I cleaned this up to the extent I found reasonable at the moment. @fingolfin : Do you take over again? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4417 +/- ##
==========================================
- Coverage 84.39% 84.37% -0.02%
==========================================
Files 659 663 +4
Lines 87183 87808 +625
==========================================
+ Hits 73575 74088 +513
- Misses 13608 13720 +112
|
|
|
||
| function test_elem(K::VarietyFunctionField) | ||
| F = representative_field(K) | ||
| P = base_ring(F)::MPolyRing |
There was a problem hiding this comment.
Why the type assertion @HechtiDerLachs ?
There was a problem hiding this comment.
To assist the reader of the code. Nothing more.
|
@HechtiDerLachs from my POV this is now good to go, please approve if you agree |
fingolfin
changed the title
VarietyFunctionField, remove base_ring(::VarietyFunctionField) methodVarietyFunctionField, add base_ring_type method
|
|
||
| function test_elem(K::VarietyFunctionField) | ||
| F = representative_field(K) | ||
| P = base_ring(F)::MPolyRing |
There was a problem hiding this comment.
To assist the reader of the code. Nothing more.
|
@aaruni96 : Some weird polymake trouble seems to show up in the tests here. Do you have any suggestions what to do about it? Thx! |
|
From what I can tell, it errored out while trying to create a certain file, and that lead to failure. The file already exists, and is read-only, and was last modified on 10th November. This raises an interesting question : if the julia depot is defined to be something else ( @benlorenz any idea ? |
Are you sure that the depot is properly set? and further down I see which doesn't look like a custom depot is used. |
|
I have just restarted all the macos runners, and reset |
The alternative to removing the
base_ringmethod would be to add abase_ring_typemethod (that's what the first commit in this PR does; the second then removes it andbase_ring).This would benefit from a better
test_elemimplementation, perhaps @HechtiDerLachs can help out with that.