Implemented error when incorrect bounds cause discrete_log to give incorrect answer #39484
Conversation
|
Thanks for fixing this, but I have a few minor comments/corrections. In sagemath, the keyword Please put The python style guide (PEP8) specifies that variable names should be lowercase (with underscores between the words). So you should change Generally speaking, comments should be complete sentences (this is in PEP8), so it would be better to change "Stores" to "Store" in line 911. This is not your mistake, but the error message in the final line can be incorrect, because it prints |
|
Documentation preview for this PR (built with commit cc680ff; changes) is ready! 🎉 |
…lso removed whitespace at end of line
… provided, fixed failing doctests
|
Sorry I didn't notice earlier: you need a blank line after line 893 ("Verify that :issue:`38316` is fixed::"). In a docstring, there is always a blank line between the comment that ends with a double-colon and the following doctest. Other than that, I think everything is good if the CI finds no problems. |
|
The failures on required CI Build & Test are: I don't attribute either of these to the PR, because I think plural often has failures like this, and the failure in |
sagemathgh-39484: Implemented error when incorrect bounds cause discrete_log to give incorrect answer <!-- ^ Please provide a concise and informative title. --> <!-- ^ Don't put issue numbers in the title, do this in the PR description below. --> <!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method to calculate 1 + 2". --> <!-- v Describe your changes below in detail. --> <!-- v Why is this change required? What problem does it solve? --> <!-- v If this PR resolves an open issue, please link to it here. For example, "Fixes sagemath#12345". --> Fixes sagemath#38316 . When the bounds provided to the discrete_log function did not contain the correct answer the function would sometimes return an incorrect answer. This was fixed by adding the parameter 'check' to the function. This parameter is True by default. If 'check' is True then the function verifies the answer, returning it if it is correct and throwing a Value Error if it is not. If 'check' is False then the function returns the (possibly incorrect) answer without verifying it. A doctest was added to verify that the function correctly throws an error in a situation where it previously returned an incorrect answer. ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [X] The title is concise and informative. - [X] The description explains in detail what this PR is about. - [X] I have linked a relevant issue or discussion. - [X] I have created tests covering the changes. - [X] I have updated the documentation and checked the documentation preview. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> <!-- - sagemath#34567: ... --> URL: sagemath#39484 Reported by: Caleb Van't Land Reviewer(s):
Fixes #38316 .
When the bounds provided to the discrete_log function did not contain the correct answer the function would sometimes return an incorrect answer.
This was fixed by adding the parameter 'check' to the function. This parameter is True by default. If 'check' is True then the function verifies the answer, returning it if it is correct and throwing a Value Error if it is not. If 'check' is False then the function returns the (possibly incorrect) answer without verifying it.
A doctest was added to verify that the function correctly throws an error in a situation where it previously returned an incorrect answer.
📝 Checklist
⌛ Dependencies