Add more QQFieldElem and ZZRingElem ccalls #1866
Conversation
- add some missing add!, mul!, etc. methods - add more efficient +, -, * methods for a ZZRingElem or QQFieldElem as first argument, and an integer as second argument - optimize is_one and is_zero for QQFieldElem - drop some pointless uses of `@inline` - add `cmp` for QQFieldElem and use it - add numerator! and denominator! - change a bunch of "unsafe" methods to accept ZZRingElemOrPtr resp. QQFieldElemOrPtr instead of just ZZRingElem and QQFieldElem to make them more flexible
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1866 +/- ##
==========================================
+ Coverage 87.00% 87.29% +0.29%
==========================================
Files 98 97 -1
Lines 35934 35878 -56
==========================================
+ Hits 31264 31321 +57
+ Misses 4670 4557 -113 ☔ View full report in Codecov by Sentry. |
| +(a::Int, b::QQFieldElem) = b + a | ||
|
|
||
| +(a::ZZRingElem, b::QQFieldElem) = b + a | ||
| +(a::QQFieldElem, b::Integer) = a + ZZRingElem(b) |
There was a problem hiding this comment.
Some idea in my head: dispatch this directly to add! and handle the conversion there (similar for the other arithmetics). Then the above loop can be removed and instead all methods be installed for IntegerUnion.
What do you think about this?
There was a problem hiding this comment.
I am OK with handling more in add!. However, we can't get rid of the loop because we'll get ambiguity errors (e.g. for FracField + Int vs QQFieldElem + Integer).
I suggest we merge this first and then we can think about handling additional cases. In fact I have another PR that helps with that in the pipeline.
| return c | ||
| end | ||
|
|
||
| add!(c::QQFieldElemOrPtr, a::Union{ZZRingElemOrPtr, Int, UInt}, b::QQFieldElemOrPtr) = add!(c, b, a) |
There was a problem hiding this comment.
With the proposed change above (doing conversion in add! instead of +), this should get changed to IntegerUnion as well, to only have to do conversion in the last argument
If we don't apply the above change, I would argue that this method doesn't have a right to stay, as we only allow restricted types in these functions.
Similarly, sub! doesn't handle the wrong argument order
There was a problem hiding this comment.
The case of sub! is a bit different, because there we can't just switch the order without loss -- we have to insert a neg!
There was a problem hiding this comment.
Apart from #1866 (comment), I am happy to merge this first and discuss later, before we get any problems with conflicts or downstream
Co-authored-by: Lars Göttgens <[email protected]>
|
For the OscarCI failure, I think you can add a method for QQFieldElem cmp that converts Integer to ZZRingElem |
Done. |
|
The OscarCI tests are running but I just noticed a bunch of precompilation errors: (https://github.com/Nemocas/Nemo.jl/actions/runs/11047475910/job/30688836009?pr=1866#step:7:305) Triggered from here: https://github.com/thofma/Hecke.jl/blob/master/src/QuadForm/Enumeration.jl#L1028 |
|
Ah, that's because those functions also exist in Hecke. I'll take them out here for now and we can migrate them later |
|
How about keeping them, but not exporting them? |
|
@thofma ah that might work, good idea, thank you |
@inlinecmpfor QQFieldElem and use itThis got a bit out of hand. I have more things lined up, but let's get this in while it passes my local tests, and to see what HeckeCI and OscarCI report.