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

Lattes to curve function code commits for enhancement #38329 #38330

Merged
merged 17 commits into from
Aug 10, 2024

Conversation

Nathabolin
Copy link
Contributor

@Nathabolin Nathabolin commented Jul 2, 2024

📝 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

@Nathabolin Nathabolin changed the title Lattes to curve function code commits for enhancement #38329 Lattes to curve function Jul 5, 2024
@bhutz
Copy link

bhutz commented Jul 8, 2024

A number of simple things to fix here:

  • 6843: Finds a short Weierstrass model associated to this Lattes map
  • 6844 - extra white space
  • 6845 self is assumed to be
  • 6848, 6852, 6853 self
  • 6866, 6882, 6895 white space between arguments
  • 6875 extra blank line
  • 6911, 6920, 6928, 6934, 6942 fix indent
  • 6914,6915,6916 extra white space
  • 6918 extra \
  • 6912-25, 6929 spacing needed

Less simple: I think you should add at least one doc tests that checks the output is correct. Something like

E, m = F.Lattes_to_curve()
P.Lattes_map(E,2).conjugate(m) == F

Finally, you cannot in general assume that u=1. The problem you are running into in the more general case is that the solution is not defined over QQ, not that there isn't a solution.

@bhutz
Copy link

bhutz commented Jul 12, 2024

I did a full functionality test and found a couple things. There are a couple more minor coding things too.

  • need to wrap the very long output lines in the doctests
  • and the defining polynomial of any required extension is not actually part of the output. That is just part of the elliptic curve definition, so remove that phrase.
  • move imports to top of file
  • 6939-40 These lines are not used
P = ProjectiveSpace(self.base_ring(), 1, "x, y")
 x, y = P.gens()
  • I would suggest a parameter that defaults to true check_Lattes=True. If that parameter is true, then call is_Lattes after the degree check (6920). Add a test against a simple map
P.<x,y>=ProjectiveSpace(QQ,1)
f=DynamicalSystem([x^4,y^4])
f.Lattes_to_curve()
  • There are some issues over number fields. The following fails due to a naming conflict on the variables
K.<a>=QuadraticField(2)
P.<x,y>=ProjectiveSpace(K,1)
E=EllipticCurve([1,a])
f=P.Lattes_map(E,2)
print(f)
f.Lattes_to_curve()

I would suggest renaming the variables to something no-one would typically use such as

    a = pts[0]['avar']
    b = pts[0]['bvar']
    u = pts[0]['uvar']
    v = pts[0]['vvar']
    t = pts[0]['tvar']
    w = pts[0]['wvar']

Nothing else needs to change for this since you use

a, b, u, v, w, t = R.gens()

but you do need to fix another coercion problem from this

eq += [R(g.coefficient({x:i})) - R(g_sym.coefficient({z:i}))]

Add a number field example after you fix these.

  • Similarly there are problems coercing to QQbar when the field doesn't have the coercion specified
R.<t>=QQ[]
K.<c>=NumberField(t^3-2)
P.<x,y>=ProjectiveSpace(K,1)
E=EllipticCurve([1,c])
f=P.Lattes_map(E,2)
f.Lattes_to_curve()

This can be fixed by changing the code to

This can be fixed by changing the code to do:
phi = R.base_ring().embeddings(QQbar)[0]
eq = [poly.change_ring(phi) for poly in eq]
I = R.ideal(eq)
pts = I.variety()
  • Finally: This is returning an error, but it should work:
P.<x,y>=ProjectiveSpace(QQbar,1)
E=EllipticCurve([1,2])
f=P.Lattes_map(E,2)
f.Lattes_to_curve()

so you just need to allow QQbar through in your check against numberfields

@Nathabolin
Copy link
Contributor Author

"move imports to top of file"
I'm going to do this of course but oddly enough
the file already does try to import them

lazy_import('sage.rings.qqbar', 'number_field_elements_from_algebraics')

is on line 118

@bhutz
Copy link

bhutz commented Jul 18, 2024

I having all the functionality working in this version. I still have a couple comments though.

  • The example
    """
    sage: P.<x,y> = ProjectiveSpace(QQbar,1)
    sage: F = DynamicalSystem_projective([x^4,y^4])
    sage: F.Lattes_to_curve(check_lattes=True)
    Traceback (most recent call last):
    ...
    NotImplementedError: Base ring must be a number field
    """
    is failing for is_Lattes not for Lattes_to_curve. I also don't see a reason why it doesn't allow QQbar. Why don't you make the same check in that function too.

  • Since the default is not to check if its Lattes, maybe add a check
    """
    if len(pts) == 0:
    raise ValueError("No Solutions found. Check if map is Lattes.")
    """

  • There are some white spacing issues in the later examples.

@Nathabolin Nathabolin changed the title code commits for enhancement #38329 Lattes to curve function Lattes to curve function code commits for enhancement #38329 Jul 22, 2024
@vbraun
Copy link
Member

vbraun commented Jul 24, 2024

Merge conflict

@Nathabolin
Copy link
Contributor Author

@vbraun Could you point me to where to see the merge conflicts?

@vbraun
Copy link
Member

vbraun commented Aug 4, 2024

Looks like you fixed the conflict in the last merge commit

vbraun pushed a commit to vbraun/sage that referenced this pull request Aug 5, 2024
sagemath#38329

    
- **Fixing issue sagemath#38329**
- **Adding Function with documentation to be reviewed for the above
ticket**
- **Lattes to curve function**

<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->



### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [ ] 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

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#38330
Reported by: Nathabolin
Reviewer(s):
@vbraun vbraun merged commit 66f679d into sagemath:develop Aug 10, 2024
19 checks passed
@mkoeppe mkoeppe added this to the sage-10.5 milestone Aug 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 this pull request may close these issues.

4 participants