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

Implement preimage computation for Homomorphism from Quotient Ring to Finite Field of the same characteristic #39709

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

Conversation

rasti37
Copy link

@rasti37 rasti37 commented Mar 14, 2025

Technical Details

This PR introduces a new function into the file src/sage/rings/morphism.pyx, named _preimage_from_linear_dependence.
More specifically, it computes the preimage of an element for a homomorphism $f$ from a univariate polynomial quotient ring to a finite extension field extension. When the codomain's field degree is the same as the domain's base ring degree, and the domain is constructed from an irreducible polynomial, $f$ is guaranteed to have a preimage for any element and _preimage_from_linear_dependence returns this preimage.

Fixes #39690.

The way this function is currently used, it fixes the NotImplementedError raised by:

  • f.inverse
  • f.inverse_image and
  • f._inverse_image_element

How it was fixed?

Internally, _inverse_image_element calls _graph_ideal which is the cause of the NotImplementedError due to the check A.base_ring() != B.base_ring(). Now, before calling _graph_ideal, the new method _preimage_from_linear_dependence is conditionally called and returns early, thereby avoiding the error.

I have not included any documentation because it's supposed to be used as an internal function. Let me know if it is needed.

📝 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

  • Fixing the ruff linting #39699 : Probably due to a recent version change in the ruff linter, the ruff linting check failed. This PR fixes these errors so I cherry-picked the author's commit.

@rasti37 rasti37 changed the title Compute Preimage for Homomorphism from Quotient Rings to Finite Fields of the same degree Compute preimage for Homomorphism from Quotient Ring to Finite Field of the same degree Mar 14, 2025
@rasti37 rasti37 changed the title Compute preimage for Homomorphism from Quotient Ring to Finite Field of the same degree Implement preimage computation for Homomorphism from Quotient Ring to Finite Field of the same degree Mar 14, 2025
@rasti37 rasti37 changed the title Implement preimage computation for Homomorphism from Quotient Ring to Finite Field of the same degree Implement preimage computation for Homomorphism from Quotient Ring to Finite Field of the same characteristic Mar 18, 2025
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.

I will look at it more detailedly later once my Sage rebuilds.
By the way, please avoid touching random styling issues in the future, I know it annoys you (and me) but better do it in a separate PR (and we should have more of those!)

sage: f._preimage_from_linear_dependence(z)
Traceback (most recent call last):
...
ValueError: The cardinalities of the domain (=1024) and codomain (=64) should be equal.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ValueError: The cardinalities of the domain (=1024) and codomain (=64) should be equal.
ValueError: the cardinalities of the domain (=1024) and codomain (=64) should be equal

Copy link
Author

@rasti37 rasti37 Mar 20, 2025

Choose a reason for hiding this comment

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

Done! (Check new commit)

D = self.domain()
C = self.codomain()
if (d_card := D.cardinality()) != (c_card := C.cardinality()):
raise ValueError(f"The cardinalities of the domain (={d_card}) and codomain (={c_card}) should be equal.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
raise ValueError(f"The cardinalities of the domain (={d_card}) and codomain (={c_card}) should be equal.")
raise ValueError(f"the cardinalities of the domain (={d_card}) and codomain (={c_card}) should be equal")

errors should start with lowercase and not end with punctuations

Copy link
Author

Choose a reason for hiding this comment

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

Done! (Check new commit)

raise TypeError(f"{b} fails to convert into the morphism's codomain {C}")
F1 = D.base_ring()
if F1.prime_subfield() != C.prime_subfield():
raise NotImplementedError("The domain's base ring and codomain's prime subfields should match.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
raise NotImplementedError("The domain's base ring and codomain's prime subfields should match.")
raise ValueError("the domain's base ring and codomain's prime subfields should match")

It seems to be this should be a value error?

Copy link
Author

Choose a reason for hiding this comment

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

Done! (Check new commit)

Comment on lines 1112 to 1116
from sage.rings.finite_rings.finite_field_base import FiniteField
from sage.rings.quotient_ring import QuotientRing_nc
if isinstance(self.domain(), QuotientRing_nc) and isinstance(self.codomain(), FiniteField):
if self.domain().base_ring().prime_subfield() == self.codomain().prime_subfield():
return self._preimage_from_linear_dependence(b)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a test for this part by the way?

Copy link
Author

Choose a reason for hiding this comment

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

Since _inverse_image_element is a private function, the tests inside should call a public function that eventually calls this private one (for example the rest tests use inverse_image). Is that okay?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. You can add the indirect doctest tag after the test to make the coverage test happy (does anyone care...)

Copy link
Author

Choose a reason for hiding this comment

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

Done! (Check new commit)

@grhkm21
Copy link
Contributor

grhkm21 commented Mar 20, 2025

Also, do you mind elaborating what happens if the map is not bijective but domain and codomain have the same size?

And another question, since ring homs are decided by the action on the generators, would it be more efficient if you cache the result $\beta = f^{-1}(\alpha)$ (if $\alpha$ is a generator of the domain), then $f^{-1}(\alpha^2 + \alpha + 1) = \beta^2 + \beta + 1$ (apply $f$ on both sides to see)

from sage.rings.finite_rings.finite_field_base import FiniteField
from sage.rings.quotient_ring import QuotientRing_nc
if isinstance(self.domain(), QuotientRing_nc) and isinstance(self.codomain(), FiniteField):
if self.domain().base_ring().prime_subfield() == self.codomain().prime_subfield():
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if self.domain().base_ring().prime_subfield() == self.codomain().prime_subfield():
if self.domain().base_ring().prime_subfield() is self.codomain().prime_subfield():

Sage (especially algebraic) stuff usually inherit from UniqueFactory so is works and is faster (bypasses coercion).

@grhkm21
Copy link
Contributor

grhkm21 commented Mar 20, 2025

One more comment: I realised that this PR is supposed to fix #39690. Can you reflect that in your test? In other words, add a test (or really just a line) showing that f.inverse() works.

@rasti37
Copy link
Author

rasti37 commented Mar 20, 2025

One more comment: I realised that this PR is supposed to fix #39690. Can you reflect that in your test? In other words, add a test (or really just a line) showing that f.inverse() works.

This test should be included in .inverse, ._inverse_image_element or _preimage_from_linear_dependence ?

@grhkm21
Copy link
Contributor

grhkm21 commented Mar 20, 2025

.inverse since it's the public method. IIRC methods starting with an underscore don't need tests officially

Copy link

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

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