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

Random failure in ell_field.py with division_field #36832

Closed
2 tasks done
jhpalmieri opened this issue Dec 7, 2023 · 17 comments · Fixed by #37159
Closed
2 tasks done

Random failure in ell_field.py with division_field #36832

jhpalmieri opened this issue Dec 7, 2023 · 17 comments · Fixed by #37159

Comments

@jhpalmieri
Copy link
Member

Steps To Reproduce

This raises an error:

sage: F.<a> = GF(592337197)
sage: E = EllipticCurve([F(341280459), F(489478549), F(203134672), F(127259459), F(366459012)])
sage: E
Elliptic Curve defined by y^2 + 341280459*x*y + 203134672*y = x^3 + 489478549*x^2 + 127259459*x + 366459012 over Finite Field of size 592337197
sage: E.division_field(3)

As a result, this doesn't pass for me (OS X):

sage -t --long --random-seed=46499345266195225323927627332877868800 src/sage/schemes/elliptic_curves/ell_field.py

Expected Behavior

The test should pass.

Actual Behavior

An error is raised:

TypeError: unable to convert Identity endomorphism of Finite Field of size 592337197 to an element of Finite Field of size 592337197

Additional Information

No response

Environment

- OS X 13.6.1, Intel, plus various `homebrew` packages
- Sage version 10.3.beta0

Checklist

  • I have searched the existing issues for a bug report that matches the one I want to file, without success.
  • I have read the documentation and troubleshoot guide
@dimpase
Copy link
Member

dimpase commented Dec 8, 2023

@JohnCremona

@DaveWitteMorris
Copy link
Member

This seems to be a new bug. I did not get an error (and the specified doctests passed) with v10.2 on intel MacOS 13.6:

sage: E.division_field(3)
Finite Field of size 592337197

But I get the TypeError now that I have upgraded to 10.3b0.

@JohnCremona
Copy link
Member

I will take a look. Not today

@dimpase
Copy link
Member

dimpase commented Dec 9, 2023

Probably related to Flint upgrade in #35848

This was referenced Dec 15, 2023
@mkoeppe
Copy link
Member

mkoeppe commented Dec 20, 2023

This failure is not specific to macOS. Also shows up randomly in the conda CI on Linux - https://github.com/sagemath/sage/actions/runs/7259211723/job/19837970698?pr=36877#step:11:8314

@yyyyx4
Copy link
Member

yyyyx4 commented Jan 8, 2024

Minimal failing example:

sage: F = GF(11)
sage: phi = Hom(F,F).an_element()
sage: R.<x,y> = F[]
sage: x.change_ring(phi)

Part of the issue is that identity finite-field embeddings are returned as a generic IdentityMorphism:

sage: type(Hom(GF(11), GF(11)).an_element())
<class 'sage.categories.morphism.IdentityMorphism'>
sage: type(Hom(GF(11), GF(11^2)).an_element())
<class 'sage.rings.finite_rings.hom_prime_finite_field.FiniteFieldHomomorphism_prime'>

...which then proceeds to cause trouble somewhere in the coercion system.

@tornaria
Copy link
Contributor

Can you add division_field to the title. I reported this in #35936 because this one didn't pop up in search.

My example is:

EllipticCurve(GF(528238639), [78525647, 395630016, 427518312, 245441800, 351879008]).division_field(3)

@jhpalmieri jhpalmieri changed the title Random failure in ell_field.py Random failure in ell_field.py with division_field Jan 23, 2024
@kwankyu
Copy link
Collaborator

kwankyu commented Jan 24, 2024

Minimal failing example:

sage: F = GF(11)
sage: phi = Hom(F,F).an_element()
sage: R.<x,y> = F[]
sage: x.change_ring(phi)

What do you mean? Is this supposed to work? phi is not a ring. So failure is expected.

@yyyyx4
Copy link
Member

yyyyx4 commented Jan 24, 2024

The input for .change_ring() can be either a ring or a ring morphism. The documentation says "R" -- a ring or morphism and shows an example where it works. In this case it also works for ring morphisms which aren't the IdentityMorphism.

(Edit) Example for your convenience:

sage: F = GF(11)
sage: phi = Hom(F, F.extension(2)).an_element()
sage: R.<x,y> = F[]
sage: x.change_ring(phi)
x
sage: x.change_ring(phi).parent()
Multivariate Polynomial Ring in x, y over Finite Field in z2 of size 11^2

@kwankyu
Copy link
Collaborator

kwankyu commented Jan 24, 2024

It makes sense. Thanks.

Then it remains to spot the code that is responsible in dealing with the morphism input.

@yyyyx4
Copy link
Member

yyyyx4 commented Jan 24, 2024

I think we should also consider the possibility that having a generic IdentityMorphism class is not very useful and it'd be better to instead construct an identity morphism of the same type as all the other morphisms in that category.

@kwankyu
Copy link
Collaborator

kwankyu commented Jan 24, 2024

Generic IdentityMorphism class itself has no fault, but the identity morphism in Hom(GF(11), GF(11)) should be a ring homomorphism, that is, an instance of RingHomomorphism or something. This seems a bug of Hom.

@kwankyu
Copy link
Collaborator

kwankyu commented Jan 24, 2024

I traced the code. At some point, isinstance(x, morphism.RingHomomorphism_im_gens) returns False for x being Identity endomorphism of Finite Field of size 11 and type(x) <class 'sage.categories.morphism.IdentityMorphism'>

@kwankyu
Copy link
Collaborator

kwankyu commented Jan 24, 2024

--- a/src/sage/rings/polynomial/multi_polynomial.pyx
+++ b/src/sage/rings/polynomial/multi_polynomial.pyx
@@ -878,9 +878,9 @@ cdef class MPolynomial(CommutativePolynomial):
             [x*y]
         """
         if isinstance(R, Map):
-        #if we're given a hom of the base ring extend to a poly hom
+            # if we're given a hom of the base ring, extend to a hom of the polynomial ring
             if R.domain() == self.base_ring():
-                R = self.parent().hom(R, self.parent().change_ring(R.codomain()))
+                R = self.parent().hom(self.parent().change_ring(R.codomain()), base_map=R)
             return R(self)
         else:
             return self.parent().change_ring(R)(self.dict())

This seems to fix the original bug (without "fixing identity morphism").

@enriqueartal
Copy link
Contributor

Can it be related to #36472?

@kwankyu
Copy link
Collaborator

kwankyu commented Jan 25, 2024

No. That is for ideals. This is for polynomials.

@kwankyu
Copy link
Collaborator

kwankyu commented Jan 25, 2024

The fix #37159 is ready for review.

vbraun pushed a commit to vbraun/sage that referenced this issue Feb 1, 2024
    
<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->

Fixes sagemath#36832.

<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

- [x] The title is concise, informative, and self-explanatory.
- [ ] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#37159
Reported by: Kwankyu Lee
Reviewer(s): Gonzalo Tornaría, Kwankyu Lee
@mantepse mantepse mentioned this issue May 10, 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 a pull request may close this issue.

9 participants