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

Implemented error when incorrect bounds cause discrete_log to give incorrect answer #39484

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

pyrusbrawler64
Copy link
Contributor

@pyrusbrawler64 pyrusbrawler64 commented Feb 10, 2025

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

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation and checked the documentation preview.

⌛ Dependencies

@DaveWitteMorris
Copy link
Member

Thanks for fixing this, but I have a few minor comments/corrections.

In sagemath, the keyword check is reserved for specifying whether to verify that the input is valid, so you should not use it for verifying the output. I don't know whether there is an established name for this parameter, but I think verify is reasonable, and it is what the sage_input function uses.

Please put *, before the new parameter in the signature of discrete_log. (Combined with the previous comment, I am asking you to change check=True to *, verify=True, or whatever new name has been chosen for the parameter.) The asterisk makes all subsequent parameters "keyword-only". There are already so many positional parameters that I don't think it makes much sense to add another one.

The python style guide (PEP8) specifies that variable names should be lowercase (with underscores between the words). So you should change originalA to original_a.

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 a, rather than original_a, so please fix it. While you are at it, I think it would be good to improve the message by including the bounds (if any). I suggest something like:

	with_bounds = f" with bounds {bounds}" if bounds else ""
	raise ValueError(f"no discrete log of {a_original} found to base {base}{with_bounds}")

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.

raise ValueError when discrete_log does not exist
2 participants