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

throw ValueError instead of TypeError when logarithm doesn't exist in AdditiveAbelianGroupWrapper #37142

Conversation

yyyyx4
Copy link
Member

@yyyyx4 yyyyx4 commented Jan 22, 2024

Attempting to run the higher-level algorithms in the additive_abelian_wrapper.py file on a group with unhashable elements will also throw a TypeError, which leads to very strange behavior (rather than failing with a clear error message, as it should).

#sd123

@grhkm21
Copy link
Contributor

grhkm21 commented Jan 23, 2024

LGTM

But I don't have review/merge perms.

@roed314
Copy link
Contributor

roed314 commented Jan 24, 2024

@grhkm21, I've invited you to the Sage organization (and the Triage team). Once you accept, you can review PRs.

@yyyyx4 yyyyx4 requested a review from grhkm21 January 25, 2024 21:22
Copy link
Contributor

@grhkm21 grhkm21 left a comment

Choose a reason for hiding this comment

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

Doctests are failing. For example, line 110 (P1 in G) throws an error now even though it should return False.

Maybe this line should be modified to catch ValueError too?

@yyyyx4 yyyyx4 force-pushed the public/throw_ValueError_when_logarithm_does_not_exist branch from 8b38164 to fecafde Compare January 27, 2024 21:22
@yyyyx4
Copy link
Member Author

yyyyx4 commented Jan 27, 2024

Thanks, and sorry, I ripped this out of a branch with a larger set of changes and apparently forgot to run the tests again prior to making the pull request...

...because attempting to run this with unhashable elements throws the
same exception, which can cause confusion in higher-level algorithms.
@yyyyx4 yyyyx4 force-pushed the public/throw_ValueError_when_logarithm_does_not_exist branch from fecafde to 4d6f31b Compare January 28, 2024 10:17
Copy link

Documentation preview for this PR (built with commit 4d6f31b; changes) is ready! 🎉

@yyyyx4 yyyyx4 requested a review from grhkm21 January 30, 2024 14:45
@grhkm21
Copy link
Contributor

grhkm21 commented Jan 30, 2024

LGTM :D

@vbraun vbraun merged commit 203431c into sagemath:develop Feb 2, 2024
17 of 19 checks passed
@yyyyx4 yyyyx4 deleted the public/throw_ValueError_when_logarithm_does_not_exist branch February 3, 2024 12:58
@mkoeppe mkoeppe added this to the sage-10.3 milestone Mar 7, 2024
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.

None yet

5 participants