Introduce flintify helper for "optimal" dispatch on integer and rational inputs
#1867
Conversation
|
|
||
| -(a::Integer, b::QQMPolyRingElem) = -(b - a) | ||
| -(a::Integer, b::QQMPolyRingElem) = neg!(b - a) |
There was a problem hiding this comment.
Ah, that's an optimization that slipped in here by accident (I was splitting this PR from other work). I'll leave it here (no harm in it, I think) but if we decide against this approach, we'd still want this change in another PR
There are a bunch of FLINT function that come in multiple varieties.
E.g. fmpz_add, fmpz_add_ui, fmpz_add_si all take one fmpz and one
additional argument of type fmpz / UInt / Int, respectively.
So naturally we define different `+` and `add!` methods in Julia using
them. But which method to invoke when adding a `ZZRingElem` and a Julia
integer of a type different from `UInt` and `Int`, such as e.g. `UInt32`
? In that case we currently always convert that into a `ZZRingElem`. But
that's inefficient, it would be be better to convert that value to a
`UInt` or `Int` value and then use the optimized `fmpz_add_ui/_si`
method.
Of course this can be done, but getting it right is tedious, repetitive
and it is easy to overlook a case.
This is where `FlintInt` comes in: this is defined as `Union{Int,
ZZRingElem}`. We then provide constructors which convert any `Integer`
to a `FlintInt` in an optimal way (at least for a bunch of built-in
integer types).
This then can be used to write optimal dispatch for Integer types
like this:
add!(x::ZRingElem, y::ZRingElem) = ...
add!(x::ZRingElem, y::Int) = ...
add!(x::ZRingElem, y::UInt) = ...
# fallback code
add!(x::ZRingElem, y::Integer) = add!(x, FlintInt(y))
It also works for types that accept ZZRingElem and Int, but not UInt:
add!(x::ZPolyRingElem, y::ZPolyRingElem) = ...
add!(x::ZPolyRingElem, y::ZRingElem) = ...
add!(x::ZPolyRingElem, y::Int) = ...
# fallback code
add!(x::ZPolyRingElem, y::Integer) = add!(x, FlintInt(y))
Of course I might have missed optimal conversion for some `Integer`
subtype. But then we can fix this by simply adding another `FlintInt`
constructor in a single central place.
Finally, the new type `FlintRat` plays a similar role for FLINT
functions which also take a `QQFieldElem`. While `RationalUnion` is a
counterpart to `IntegerUnion`.
fingolfin
force-pushed
the
mh/FlintInt-FlintRat
branch
from
dc62a31
to
b8c8bfe
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1867 +/- ##
==========================================
+ Coverage 87.00% 87.36% +0.35%
==========================================
Files 98 97 -1
Lines 35934 35888 -46
==========================================
+ Hits 31264 31352 +88
+ Misses 4670 4536 -134 ☔ View full report in Codecov by Sentry. |
|
I like the idea, but I am not sure that overloading |
Co-authored-by: Lars Göttgens <[email protected]>
having a function |
|
Yes I agree with @thofma's point. I'll rewrite this to use a function |
fingolfin
changed the title
flintify helper for "optimal" dispatch on integer and rational inputs
|
Renaming is done now |
There are a bunch of FLINT function that come in multiple varieties. E.g. fmpz_add, fmpz_add_ui, fmpz_add_si all take one fmpz and one additional argument of type fmpz / UInt / Int, respectively.
So naturally we define different
+andadd!methods in Julia using them. But which method to invoke when adding aZZRingElemand a Julia integer of a type different fromUIntandInt, such as e.g.UInt32? In that case we currently always convert that into aZZRingElem. But that's inefficient, it would be be better to convert that value to aUIntorIntvalue and then use the optimizedfmpz_add_ui/_simethod.Of course this can be done, but getting it right is tedious, repetitive and it is easy to overlook a case.
This is where
FlintIntcomes in: this is defined asUnion{Int, ZZRingElem}. We then provide constructors which convert anyIntegerto aFlintIntin an optimal way (at least for a bunch of built-in integer types).This then can be used to write optimal dispatch for Integer types like this:
It also works for types that accept ZZRingElem and Int, but not UInt:
Of course I might have missed optimal conversion for some
Integersubtype. But then we can fix this by simply adding anotherFlintIntconstructor in a single central place.Finally, the new type
FlintRatplays a similar role for FLINTfunctions which also take a
QQFieldElem. WhileRationalUnionis acounterpart to
IntegerUnion.Note that there are tons more places that could use
FlintInt, in particular I'd like to use it to make more genericadd!methods for e.g.ZZRingElemand many more (see also this comment thread started by @lgoettgens).But before I invest more work into this, I'd like to hear what others think about this approach. If we decide against it, no point in wasting more effort on it.