Skip to content
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

[DO NOT MERGE] Change FLINT's matrix structs to accomodate v3.3.0 #1999

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

albinahlback
Copy link
Contributor

Since v3.3.0, they rely on stride instead of pointers to rows.

There are still some unsafe things that has to be fixed, I didn't know how to fix them.

Since v3.3.0, they rely on stride instead of pointers to rows
@lgoettgens
Copy link
Collaborator

I think it would make sense to have some branch flint-3.3.0 where we can merge such changes as this PR into, and furthermore regularly merge the current master into there as well. Then you could use that branch for CI in flintlib/flint and still get access to other changes we do in Nemo.

ping @fingolfin

@lgoettgens
Copy link
Collaborator

There are still some unsafe things that has to be fixed, I didn't know how to fix them.

We can have a look together sometime this week

@albinahlback
Copy link
Contributor Author

You can check the remaining things by grepping for \.rows\b. The two places to fix are in fmpz_mat.jl and nmod_mat.jl, but they seem pretty unused.

@lgoettgens
Copy link
Collaborator

The one in fmpz_mat.jl is in _very_unsafe_convert introduced by @thofma.
If I understand the code there correctly, it tries to construct a fmpz_mat object from a given julia vector of fmpz. I think we could just remove the pointer arithmetic for .rows there and instead set stride = ncols.

The other one is in AbstractAlgebra.Strassen.apply! by @fieker.
This code tries to permute rows of an fmpz_mat by just permuting the pointers in rows, not doing anything about the entries themselves. @albinahlback I believe this feature is removed with the refactoring of matrices in FLINT, right? If yes, we (or rather @fieker) would need to look how to adapt the code here to not rely on that feature.

@fingolfin
Copy link
Member

The code in AbstractAlgebra.Strassen.apply! for fpMatrix in Nemo.jl is an optimization; it could simply be commented out for now, so the generic code using swap_rows! is used.

But it would still be possible (and desirable) to have an optimized version, and perhaps also for more matrix types: essentially, this function applies a permutation to the rows of the input matrix. We can still do that a lot more efficiently than via swaps if we have a buffer for one row available and otherwise use memcpy or memmove to copy rows around.

Ideally FLINT would offer an API for applying such a permutation, then we'd not have to mess with the internals. But AFAIK this doesn't exist, just for "reversing" all rows and for swapping two rows?

@fingolfin
Copy link
Member

Note that Hecke also may need some adjustments, e.g. its struct VeryBad -- and ideally we'd create APIs in Nemo that would allow Hecke to not do such things anymore (i.e., have Nemo do them instead, so we have one place to adjust to breaking FLINT changes).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants