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

Minor doc fixes (one requires verification) #2051

Merged
merged 3 commits into from
Aug 23, 2024

Conversation

vneiger
Copy link
Collaborator

@vneiger vneiger commented Aug 23, 2024

These are minor doc fixes.

One of these fixes needs verification (@albinahlback ?). The current doc says that for udiv_qrnnd, the input should satisfy d < n_1, but after seeing several use cases of this function, including its test file, it seems that this is a typo: should it rather be n_1 < d? I have not checked this carefully, but conjectured the answer is yes and modified the doc in this direction.

@vneiger
Copy link
Collaborator Author

vneiger commented Aug 23, 2024

Related (remarks/questions also directed to @albinahlback since you were the one touching this part of flint recently): does udiv_qrnnd_preinv only work for d that has msb set to 1? it seems to be the case at least in the test and profile files, and very quick tests with smaller d's seem to confirm that this fails. But this is not specified in the doc or in the relevant .h/.c files. If yes, is there any easy way to make it work more generally? It is quite faster than udiv_qrrnd and may at least slightly impact some nmod_mat and nmod_vec things. If no, then I will use this PR to update the documentation to mention this requirement on d.

(Let me know if I should rather open an issue for this.)

@albinahlback
Copy link
Collaborator

One of these fixes needs verification (@albinahlback ?). The current doc says that for udiv_qrnnd, the input should satisfy d < n_1, but after seeing several use cases of this function, including its test file, it seems that this is a typo: should it rather be n_1 < d?

Yes, that is correct. Thanks for spotting this!

@albinahlback
Copy link
Collaborator

Related (remarks/questions also directed to @albinahlback since you were the one touching this part of flint recently): does udiv_qrnnd_preinv only work for d that has msb set to 1? it seems to be the case at least in the test and profile files, and very quick tests with smaller d's seem to confirm that this fails. But this is not specified in the doc or in the relevant .h/.c files. If yes, is there any easy way to make it work more generally? It is quite faster than udiv_qrrnd and may at least slightly impact some nmod_mat and nmod_vec things. If no, then I will use this PR to update the documentation to mention this requirement on d.

Yes, so I believe the algorithm is Granlund-Möller's division algorithm. As such, it assumes a normalized divisor.

To make it work in a more general case, simply shift both numerator and denominator so that divisor is normalized.

And yes, it looks like I screwed up the docstrings when I touched them.

@albinahlback
Copy link
Collaborator

As you say, we should say something about normalization in udiv_qrnnd_preinv. Moreover, the docstring for n_preinvert_limb_prenorm needs a rework. It doesn't utilize the algorithm in Granlund-Montgomery, but rather the later Granlund-Möller. We should open a joint issue about this.

@vneiger
Copy link
Collaborator Author

vneiger commented Aug 23, 2024

Related (remarks/questions also directed to @albinahlback since you were the one touching this part of flint recently): does udiv_qrnnd_preinv only work for d that has msb set to 1? it seems to be the case at least in the test and profile files, and very quick tests with smaller d's seem to confirm that this fails. But this is not specified in the doc or in the relevant .h/.c files. If yes, is there any easy way to make it work more generally? It is quite faster than udiv_qrrnd and may at least slightly impact some nmod_mat and nmod_vec things. If no, then I will use this PR to update the documentation to mention this requirement on d.

Yes, so I believe the algorithm is Granlund-Möller's division algorithm. As such, it assumes a normalized divisor.

To make it work in a more general case, simply shift both numerator and denominator so that divisor is normalized.

Ah, right, thanks. I have added a word about the normalization requirement in the doc (similar to that for mulmod_preinv).

@albinahlback
Copy link
Collaborator

When you are happy, please merge!

@vneiger
Copy link
Collaborator Author

vneiger commented Aug 23, 2024

When you are happy, please merge!

Alright, and thanks for your help! (Related small changes coming, concerning mulmod_shoup: better done in another PR.)

@vneiger vneiger merged commit 338df24 into flintlib:main Aug 23, 2024
10 checks passed
@vneiger
Copy link
Collaborator Author

vneiger commented Aug 23, 2024

As you say, we should say something about normalization in udiv_qrnnd_preinv. Moreover, the docstring for n_preinvert_limb_prenorm needs a rework. It doesn't utilize the algorithm in Granlund-Montgomery, but rather the later Granlund-Möller. We should open a joint issue about this.

I opened #2052 , feel free to add other things you had in mind regarding this point.

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

Successfully merging this pull request may close these issues.

2 participants