-
Notifications
You must be signed in to change notification settings - Fork 58
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce low-level analogs for Flint structs #1889
base: master
Are you sure you want to change the base?
Conversation
@@ -42,7 +44,7 @@ integer_ring() = ZZRing() | |||
|
|||
@doc zz_ring_doc | |||
mutable struct ZZRingElem <: RingElem | |||
d::Int | |||
d::Flint.fmpz |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
d::Flint.fmpz | |
data::Flint.fmpz |
consistency with the other structs
Could you provide your regexes here? I'd like to play a bit with them around |
This does not cover arrays (for no deep reason other than that I only encountered one instances of those, and late, so I did it manually) |
norm::flint_bitcnt_t | ||
end | ||
|
||
struct fmpz_poly_struct |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Things I have no thought through yet: which of these should we do (and why):
fmpz_poly_struct
astruct
, andZZPolyRingElem
amutable struct
with finalizer (this is what we have now)fmpz_poly_struct
amutable struct
, with finalizer, andZZPolyRingElem
astruct
- both
mutable struct
(and one of them has finalizer)
I did 1 because this needed the least number of changes and this PR is just an incomplete PoC
Pro for 2 would be that several high-level Julia structs wrap the same underlying FLINT structs (e.g. fmpz_poly_struct
is wrapped by ZZPolyRingElem
, ZZAbsPowerSeriesRingElem
and ZZRelPowerSeriesRingElem
) and so they could use the same finalizer (well technically they already could do that now).
Anyway, I did not try methods 2 and 3, they might not work at all for fundamental reasons, or maybe method 1 (what this PR has now) is fundamentally flawed -- gotta find some time to experiment more. But if someone else has insights, please share them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would find it most intuitive if the finalized is attached to the fmpz_poly_struct as that is the most fundamental data structure that occupies foreign memory. And as you pointed out, that removes some duplication between multiple Nemo types using the same flint primitives
This is starting work towards resolving #1875.
I have a few regex with which I can copy&past C struct definitions from flint headers and quickly turn them into Julia structs. However on the long run it might be better to write a little script that you point at the Flint headers in extracts everything for us automatically, that way we can re-run it on Flint updates.
Unfortunately Hecke accesses a few member variables of various of our Flint wrappers directly (e.g.
mod_n
,ninv
), so we need to coordinate this carefully. Perhaps by adding some getter functions for those first, then changing Hecke to use those, then we can merge this (well, once it is ready).What this does not yet show are:
add!
,set!
to also accept these C types directlymat_entry_ptr
should returnPtr{fmpz_poly_struct}
instead ofPtr{ZZPolyRingElem}
etc.BlahAbsPowerSeriesRingElem
into a single parametrized one, and like wise for relative ones (those are basically wrappers around Flint polynomials, and we can re-use the sameset!
,add!
etc. methods on them)