Skip to content

Fix lift_x() for characteristic 2 and add is_x_coord() to hyperelliptic curves #37126

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 9 commits into from
Feb 2, 2024
Merged

Fix lift_x() for characteristic 2 and add is_x_coord() to hyperelliptic curves #37126

merged 9 commits into from
Feb 2, 2024

Conversation

GiacomoPope
Copy link
Contributor

@GiacomoPope GiacomoPope commented Jan 21, 2024 •
edited
Loading

This PR fixes #37097, where the lifting of x-coordinates of hyperelliptic curves defined over fields of characteristic two would fail.

It additionally includes a new method is_x_coord(x), which returns a bool based on whether the input x is a valid x-coordinate for the base field of which the hyperelliptic curve is defined.

While simplifying code, a bug was found computing roots in QQ, were an error was raised instead of returning an empty list when all=True. As a result, this PR also fixes #37153.

#sd123

📝 Checklist

  • The title is concise, informative, and self-explanatory.
  • 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 accordingly.

@GiacomoPope GiacomoPope changed the title Fix lift_x for characteristic 2 and add is_x_coord() to hyperelliptic curves Fix lift_x() for characteristic 2 and add is_x_coord() to hyperelliptic curves Jan 21, 2024
yyyyx4
yyyyx4 reviewed Jan 23, 2024
View reviewed changes
@GiacomoPope
Copy link
Contributor Author

Thanks for the review!

yyyyx4
yyyyx4 reviewed Jan 23, 2024
View reviewed changes
@GiacomoPope
Copy link
Contributor Author

Following issues #9466 and #26509, I have modified the sqrt() for rationals such that when all=True no errors are raised, but instead empty lists are returned. This allows for the optimisation mentioned by @yyyyx4 to be included, skipping is_square().

@GiacomoPope
Copy link
Contributor Author

GiacomoPope commented Jan 23, 2024 •
edited
Loading

To ensure that the QQ bug can be properly tracked, I made a new issue: #37153.

This PR fixes this issue as well as the first encountered bug.

@github-actions GitHub Actions
Copy link

Documentation preview for this PR (built with commit 54a212e; changes) is ready! 🎉

@GiacomoPope
Copy link
Contributor Author

Is this PR being held by the failures in the above tests? As far as I can tell the failures seem orthogonal to the changes I have made here.

grhkm21
grhkm21 approved these changes Jan 26, 2024
View reviewed changes
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.

No, I think simply no one reviewed it. Positive review!

vbraun pushed a commit to vbraun/sage that referenced this pull request Jan 27, 2024
4474f85 
sagemathgh-37126: Fix `lift_x()` for characteristic 2 and add `is_x_coord()` to hyperelliptic curves
    
This PR fixes sagemath#37097, where the lifting of x-coordinates of
hyperelliptic curves defined over fields of characteristic two would
fail.

It additionally includes a new method `is_x_coord(x)`, which returns a
`bool` based on whether the input `x` is a valid x-coordinate for the
base field of which the hyperelliptic curve is defined.

While simplifying code, a bug was found computing roots in `QQ`, were an
error was raised instead of returning an empty list when `all=True`. As
a result, this PR also fixes sagemath#37153.

#sd123

### 📝 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.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [x] I have updated the documentation accordingly.
    
URL: sagemath#37126
Reported by: Giacomo Pope
Reviewer(s): Giacomo Pope, grhkm21, Lorenz Panny
vbraun pushed a commit to vbraun/sage that referenced this pull request Jan 29, 2024
38569b3 
sagemathgh-37126: Fix `lift_x()` for characteristic 2 and add `is_x_coord()` to hyperelliptic curves
    
This PR fixes sagemath#37097, where the lifting of x-coordinates of
hyperelliptic curves defined over fields of characteristic two would
fail.

It additionally includes a new method `is_x_coord(x)`, which returns a
`bool` based on whether the input `x` is a valid x-coordinate for the
base field of which the hyperelliptic curve is defined.

While simplifying code, a bug was found computing roots in `QQ`, were an
error was raised instead of returning an empty list when `all=True`. As
a result, this PR also fixes sagemath#37153.

#sd123

### 📝 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.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [x] I have updated the documentation accordingly.
    
URL: sagemath#37126
Reported by: Giacomo Pope
Reviewer(s): Giacomo Pope, grhkm21, Lorenz Panny
vbraun pushed a commit to vbraun/sage that referenced this pull request Jan 30, 2024
fbe9c86 
sagemathgh-37126: Fix `lift_x()` for characteristic 2 and add `is_x_coord()` to hyperelliptic curves
    
This PR fixes sagemath#37097, where the lifting of x-coordinates of
hyperelliptic curves defined over fields of characteristic two would
fail.

It additionally includes a new method `is_x_coord(x)`, which returns a
`bool` based on whether the input `x` is a valid x-coordinate for the
base field of which the hyperelliptic curve is defined.

While simplifying code, a bug was found computing roots in `QQ`, were an
error was raised instead of returning an empty list when `all=True`. As
a result, this PR also fixes sagemath#37153.

#sd123

### 📝 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.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [x] I have updated the documentation accordingly.
    
URL: sagemath#37126
Reported by: Giacomo Pope
Reviewer(s): Giacomo Pope, grhkm21, Lorenz Panny
@vbraun vbraun merged commit 5cdebae into sagemath:develop Feb 2, 2024
@GiacomoPope GiacomoPope deleted the hyperelliptic_lift_x branch March 6, 2024 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
2 more reviewers

@yyyyx4 yyyyx4 yyyyx4 left review comments

@grhkm21 grhkm21 grhkm21 approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
c: algebraic geometry p: major / 3 t: bug t: enhancement
Projects
None yet
Milestone
sage-10.3
5 participants
@GiacomoPope @grhkm21 @yyyyx4 @vbraun @mkoeppe