Skip to content

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

Closed
albinahlback wants to merge 1 commit into
Nemocas:masterfrom
albinahlback:flint_matrix_stride
Closed

albinahlback wants to merge 1 commit into
Nemocas:masterfrom
albinahlback:flint_matrix_stride

Conversation

@albinahlback

Copy link
Copy Markdown
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.

@lgoettgens

Copy link
Copy Markdown
Member

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
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
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).

Since v3.3.0, they rely on stride instead of pointers to rows
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