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

Dropping support for power with Finite Field Polynomial Element #38754

Closed
wants to merge 5 commits into from

Conversation

Shay2Shay
Copy link
Contributor

@Shay2Shay Shay2Shay commented Oct 2, 2024

Fixes issue : #32287
Reference : pow ( Int, Finite Field)
It was noticed that raising power with ModN field numbers and Gravio/NTL was not consistent.

  • Finite Field = ModN : Coerced the FF element to Integer then performed Powering
  • Finite Field = Gravio/NTL: Coerced the output to Gravio/NTL rather than Integer because it could only interpret non prime order field elements to be polynomials.

Change:
Removed support for Gravio/NTL/Pari_ffelt in powering scenario.

📝 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 Oct 2, 2024

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

@Shay2Shay Shay2Shay marked this pull request as draft October 3, 2024 04:45
@Shay2Shay Shay2Shay marked this pull request as ready for review October 4, 2024 06:43
@Shay2Shay
Copy link
Contributor Author

@tscrim please have a look.

Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

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

I don't think this is the correct way to fix the issue. Adding very special cases in generic code for deciding coercion should not be done without a very good reason. Contrast your code with the generic base class IntegerModRing. Hence, my proposed change, get every other case that is not an IntegerModRing but a finite field.

Comment on lines 2744 to 2747
from sage.rings.finite_rings.finite_field_pari_ffelt import FiniteField_pari_ffelt
from sage.rings.finite_rings.finite_field_ntl_gf2e import FiniteField_ntl_gf2e
from sage.rings.finite_rings.finite_field_givaro import FiniteField_givaro
if isinstance(S, (FiniteField_pari_ffelt, FiniteField_ntl_gf2e, FiniteField_givaro)):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
from sage.rings.finite_rings.finite_field_pari_ffelt import FiniteField_pari_ffelt
from sage.rings.finite_rings.finite_field_ntl_gf2e import FiniteField_ntl_gf2e
from sage.rings.finite_rings.finite_field_givaro import FiniteField_givaro
if isinstance(S, (FiniteField_pari_ffelt, FiniteField_ntl_gf2e, FiniteField_givaro)):
elif isinstance(S, FiniteField):

You will also need to add

from sage.rings.finite_rings.finite_field_base import FiniteField

# See Issue : #32287
# Disallowing Finite Field Polynomial Element
# to be used to raise power
raise TypeError('Unable to raise power using finite field polynomial element')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
raise TypeError('Unable to raise power using finite field polynomial element')
_record_exception()

I think this is the correct thing to do.

@Shay2Shay
Copy link
Contributor Author

I understood the 1st part elif isinstance(S, FiniteField) and I made desired changes.

But I couldn't understand the second part _record_exception(). I originally intended the function call to throw error and halt.
But removing raise error will not stop the program and at it will coerce the base of power to be finite field element - this is what I am thinking.

@tscrim
Copy link
Collaborator

tscrim commented Oct 6, 2024

Sorry, I forgot what _record_exception does; yes, that is not the proper thing to do there. Errors definitely should not be raised in that code. In turn, this made me realize something: specific coercions between specific parents should not be (dis)allowed by the coercion code. Those specific parents should be the ones to handle them. So I do not think this is the correct fix. We probably need to dig deeper into what coercions are being called and figure out a better mechanism for disallowing it.

@Shay2Shay
Copy link
Contributor Author

Shay2Shay commented Oct 6, 2024

Understood

I will close this and look for another approach.

Those specific parents should be the ones to handle them.

Like ntl - old implementation.

sage: x = 123**GF(64, impl='ntl')(5); x, x.parent()
...
...
...
TypeError: unsupported operand parent(s) for ^: 'Finite Field in z6 of size 2^6' and 'Finite Field in z6 of size 2^6'

@Shay2Shay
Copy link
Contributor Author

Shay2Shay commented Oct 7, 2024

@tscrim
If I add here (Same with other variants):

if isinstance(exp.parent(), FiniteField) and not isinstance(exp.parent(), ModNField):
    raise TypeError("Cannot raise power with polynomial")

How would that be?? (This works locally)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants