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

Add gr_mat_pow_ui and gr_mat_pow_fmpz, use it in added fmpz_mod_mat_pow_ui #2189

Merged
merged 10 commits into from
Jan 27, 2025

Conversation

lgoettgens
Copy link
Contributor

My first contribution from the workshop (I don't have the rights to add the label myself, could someone please do that for me and/or give me the appropriate rights?).

To be used in Nemo in https://github.com/lgoettgens/Nemo.jl/blob/d12b72e291400fe9609706564cdb289eee472943/src/flint/fmpz_mod_mat.jl#L270-L281.

@albinahlback
Copy link
Collaborator

I wonder if this should use GR instead? @fredrik-johansson

@fredrik-johansson
Copy link
Collaborator

Arguably there should be separate methods fmpz_mod_mat_pow_ui and fmpz_mod_mat_pow_fmpz since it makes sense to raise to a multiprecision exponent here.

I wonder if this should use GR instead? @fredrik-johansson

Currently there is no gr_mat_pow_ui or gr_mat_pow_fmpz to wrap. Until then, this is fine. (One could initialize a matrix ring and use the generic gr_pow_ui / gr_pow_fmpz, but I'd prefer to have specialized gr_mat methods for powering).

@lgoettgens
Copy link
Contributor Author

Arguably there should be separate methods fmpz_mod_mat_pow_ui and fmpz_mod_mat_pow_fmpz since it makes sense to raise to a multiprecision exponent here.

true, I'll rename the function here to *_ui and work on a fmpz version as well.

I wonder if this should use GR instead? @fredrik-johansson

Currently there is no gr_mat_pow_ui or gr_mat_pow_fmpz to wrap. Until then, this is fine. (One could initialize a matrix ring and use the generic gr_pow_ui / gr_pow_fmpz, but I'd prefer to have specialized gr_mat methods for powering).

I could look into this instead of the writing more or less the same function for different types

@lgoettgens lgoettgens changed the title Add fmpz_mod_mat_pow Add gr_mat_pow_ui, use it in added fmpz_mod_mat_pow_ui Jan 27, 2025
@fredrik-johansson
Copy link
Collaborator

I could look into this instead of the writing more or less the same function for different types

It's a good idea.

Actually, it would be interesting to compare a direct implementation of gr_mat_pow_ui / gr_mat_pow_fmpz with a version that creates a matrix ring and calls the generic gr_generic_pow_ui_binexp / gr_generic_pow_fmpz_binexp. If the performance difference is negligible with 2x2 matrices, one could just use the more generic approach to save a few lines of code.

One could also have gr_mat_pow_fmpz_sliding wrapping gr_generic_pow_fmpz_sliding. I'm not sure if this should be used by default instead of the binexp algorithm since it uses much more memory, which could be significant when working with large matrices.

@lgoettgens
Copy link
Contributor Author

I am experiencing a segfault in the fmpz_mod_mat_pow testcase (that now uses gr_mat_pow_ui). Could one of you please have a look if you spot where this could come from? Or give me a pointer on how to track the issue down? Thanks!

src/gr_mat/pow.c Outdated Show resolved Hide resolved
@fredrik-johansson
Copy link
Collaborator

Looks good. Do you want to do pow_fmpz in the same PR or should we merge this now?

@lgoettgens lgoettgens changed the title Add gr_mat_pow_ui, use it in added fmpz_mod_mat_pow_ui Add gr_mat_pow_ui and gr_mat_pow_fmpz, use it in added fmpz_mod_mat_pow_ui Jan 27, 2025
@@ -46,7 +46,7 @@ fmpz_mat_pow(fmpz_mat_t B, const fmpz_mat_t A, ulong exp)
fmpz_mat_init_set(T, A);
fmpz_mat_init(U, d, d);

for (i = ((slong) FLINT_BIT_COUNT(exp)) - 2; i >= 0; i--)
for (i = FLINT_BIT_COUNT(exp) - 2; i >= 0; i--)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cast was removed on purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

by request of @albinahlback in #2189 (comment)

@fredrik-johansson
Copy link
Collaborator

LGTM!

@fredrik-johansson fredrik-johansson merged commit 0c6cbc8 into flintlib:main Jan 27, 2025
10 of 12 checks passed
@lgoettgens lgoettgens deleted the lg/fmpz_mod_mat_pow branch January 29, 2025 09:23
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.

4 participants