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.

Sign up for GitHub

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

Jump to bottom

Add an optional third argument to OrderMod that is a multiple of the order one wants to compute #4105

Merged
merged 2 commits into from
Aug 31, 2020
Merged

Add an optional third argument to OrderMod that is a multiple of the order one wants to compute #4105

merged 2 commits into from
Aug 31, 2020

Conversation

ThomasBreuer
Copy link
Contributor

@ThomasBreuer ThomasBreuer commented Aug 24, 2020 •
edited by PaulaHaehndel
Loading

admit an optional third argument in OrderMod ...
... that is a multiple of the order one wants to compute.
Note that the current function computes Lambda( m ) as the "default multiple", and then divides this number by its prime divisors until the order is found.
If one knows a reasonable multiple in advance then the perhaps expensive computation of Lambda( m ) can be skipped.

Text for release notes

The function OrderMod now admits an optional third argument that is a multiple of the order one wants to compute.

@ThomasBreuer
admit an optional third argument in OrderMod ...
9832eb5 
... that is a multiple of the order one wants to compute.
Note that the current function computes `Lambda( m )` as the
"default multiple",
and then divides this number by its prime divisors until the
order is found.
If one knows a reasonable multiple in advance then the perhaps
expensive computation of `Lambda( m )` can be skipped.
@ThomasBreuer ThomasBreuer added kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements topic: library release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes labels Aug 24, 2020
PaulaHaehndel
PaulaHaehndel approved these changes Aug 27, 2020
View reviewed changes
Copy link
Contributor

@PaulaHaehndel PaulaHaehndel left a comment

Choose a reason for hiding this comment

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

Thanks for implementing this.

One may could add a sentence in the documentation about what happens if the given bound is incorrect.
I also would assume that release notes are needed.

@ThomasBreuer ThomasBreuer added release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes and removed release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes labels Aug 27, 2020
@ThomasBreuer
Copy link
Contributor Author

@PaulaHaehndel thanks for your suggestions.

By the way:
One of the tests failed, the last lines of output are as follows.

testing: /home/travis/build/gap-system/gap/tst/testinstall/semigrp.tst
# line 186 of 506 (36%)cannot allocate memory for thread-local data: ABORT

(I think it is not very likely that the code change is the reason for this failure.)

@coveralls
Copy link

coveralls commented Aug 27, 2020 •
edited
Loading

Coverage Status

Coverage increased (+0.0001%) to 85.755% when pulling 1636042 on ThomasBreuer:TB_OrderMod into 532a88d on gap-system:master.

@wilfwilson wilfwilson merged commit 9b8f5b7 into gap-system:master Aug 31, 2020
@ThomasBreuer ThomasBreuer deleted the TB_OrderMod branch September 1, 2020 07:48
@PaulaHaehndel PaulaHaehndel self-assigned this Feb 16, 2021
@PaulaHaehndel PaulaHaehndel changed the title admit an optional third argument in OrderMod ... The function <code class="code">OrderMod</code> now admits an optional third argument that is a multiple of the order one wants to compute. Feb 16, 2021
@PaulaHaehndel PaulaHaehndel changed the title The function <code class="code">OrderMod</code> now admits an optional third argument that is a multiple of the order one wants to compute. The function OrderMod admits an optional third argument that is a multiple of the order one wants to compute. Feb 16, 2021
@PaulaHaehndel PaulaHaehndel added release notes: added PRs introducing changes that have since been mentioned in the release notes and removed release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Feb 16, 2021
@PaulaHaehndel PaulaHaehndel removed their assignment Feb 16, 2021
@PaulaHaehndel PaulaHaehndel changed the title The function OrderMod admits an optional third argument that is a multiple of the order one wants to compute. The function OrderMod admits an optional third argument that is a multiple of the order one wants to compute Feb 17, 2021
@olexandr-konovalov olexandr-konovalov mentioned this pull request Feb 19, 2021
Closed
@olexandr-konovalov olexandr-konovalov mentioned this pull request Mar 23, 2021
Merged
@fingolfin fingolfin changed the title The function OrderMod admits an optional third argument that is a multiple of the order one wants to compute Add an optional third argument to OrderMod that is a multiple of the order one wants to compute Aug 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wilfwilson wilfwilson wilfwilson approved these changes

@PaulaHaehndel PaulaHaehndel PaulaHaehndel approved these changes

Assignees
No one assigned
Labels
kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements release notes: added PRs introducing changes that have since been mentioned in the release notes topic: library
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ThomasBreuer @coveralls @wilfwilson @PaulaHaehndel