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

FiniteField: make GF(p,n) the same as GF(p^n) #38589

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

user202729
Copy link
Contributor

@user202729 user202729 commented Aug 30, 2024

It is not uncommon for the user to accidentally call GF(p,n) while they want to call GF(p^n) or GF(p^n).

This raises an error when the user try to do so. Suggested by @videlec in #38376 (comment) .

(Actually if backwards compatibility isn't that important we can just make this equivalent to GF((p,n)). I doubt any existing code specifically relies on GF(p,n) called this way with n an integer.)

📝 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

Copy link

github-actions bot commented Aug 30, 2024

Documentation preview for this PR (built with commit c46444d; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@grhkm21
Copy link
Contributor

grhkm21 commented Sep 3, 2024

Actually I'm more in favour of treating GF(p, n, ...) as GF((p, n), ...).

@videlec
Copy link
Contributor

videlec commented Sep 3, 2024

Actually I'm more in favour of treating GF(p, n, ...) as GF((p, n), ...).

I tend to agree but the (serious) drawback is an extra type checking and hence more time needed to construct the object GF(5, 'mygen').

@user202729
Copy link
Contributor Author

user202729 commented Sep 6, 2024

I tend to agree but the (serious) drawback is an extra type checking and hence more time needed to construct the object GF(5, 'mygen').

Timing data:

sage: %time GF(5^2, "a")
CPU times: user 176 ms, sys: 9.28 ms, total: 186 ms
Wall time: 234 ms
Finite Field in a of size 5^2
sage: %time isinstance(2, (int, Integer))
CPU times: user 7 μs, sys: 1 μs, total: 8 μs
Wall time: 9.54 μs
True
sage: %time GF(5^2, "a")
CPU times: user 0 ns, sys: 259 μs, total: 259 μs
Wall time: 264 μs
Finite Field in a of size 5^2

9.54 μs versus 264 μs. So around 3-4% increase if the field is already cached.

Is it that bad? (On the other hand, small changes pile up…?)


On the other hand that might break some backwards compatibility in unexpected cases e.g. the (actual) PSL code in Sage above. (Fortunately in that case the second argument is 1 anyway, and it's an IntegerMod instead of Integer so it's safe, right?)

@user202729 user202729 changed the title FiniteField: raise error on GF(p,n) FiniteField: make GF(p,n) the same as GF(p^n) Dec 18, 2024
@user202729
Copy link
Contributor Author

I modify the code to make GF(p, n) = GF((p, n)).

Thoughts?

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.

4 participants