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

Add pull_from_function_field to curves #36592

Merged
merged 10 commits into from
Dec 6, 2023

Conversation

kwankyu
Copy link
Collaborator

@kwankyu kwankyu commented Oct 30, 2023

For an algebraic curve C, C.pull_from_function_field() provides the inverse map of C.function() between the fraction field of the coordinate ring of C and the abstract function field of C. This tightens the integration of the two isomorphic fields. Elements of the fraction field of the coordinate ring of C is represented by elements of the fraction field of the coordinate ring of the ambient affine or projective space, for user's convenience.

This is the missing feature as discussed in

https://groups.google.com/g/sage-support/c/Axkaex63f8w/m/wKL0TzmDAwAJ

The problem there can be solved by

sage: P2.<x,y,z> = ProjectiveSpace(QQ, 2)
sage: f = 2*x^5 - 4*x^3*y*z + x^2*y*z^2 + 2*x*y^3*z + 2*x*y^2*z^2+ y^5
sage: C = Curve(f)
sage: K = C.function(x/y).differential().divisor()  # canonical divisor
sage: basis = (-K).basis_function_space()
sage: Basis = [C.pull_from_function_field(f) for f in basis]
sage: phi = C.hom(Basis, P2)
sage: D = phi.image()  # conic
sage: D.degree()
2
sage: D
Closed subscheme of Projective Space of dimension 2 over Rational Field defined by:
  x^2 + x*y + 2*y*z

📝 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.

⌛ Dependencies

@kwankyu kwankyu changed the title Add pull_from_function_field Add pull_from_function_field to curves Oct 30, 2023
@kwankyu kwankyu force-pushed the add-pull-from-function-field branch 2 times, most recently from 7f01a9a to fb0ccfd Compare October 31, 2023 01:17
@kwankyu kwankyu marked this pull request as ready for review October 31, 2023 01:37
@kwankyu kwankyu force-pushed the add-pull-from-function-field branch from 581102b to 64c755e Compare November 9, 2023 12:44
@kwankyu
Copy link
Collaborator Author

kwankyu commented Nov 10, 2023

@nbruin ?

@kwankyu
Copy link
Collaborator Author

kwankyu commented Nov 14, 2023

@jhpalmieri ?

@jhpalmieri
Copy link
Member

@jhpalmieri ?

Not really my field of expertise, unfortunately. I hope @nbruin can chime in.

@kwankyu kwankyu added this to the sage-10.3 milestone Nov 14, 2023
@nbruin
Copy link
Contributor

nbruin commented Nov 17, 2023

Sorry, I don't know how to review things on github. I think you'll have quicker results if someone else does it.

@jhpalmieri
Copy link
Member

Sorry, I don't know how to review things on github. I think you'll have quicker results if someone else does it.

If you find some time, you can browse the changes at https://github.com/sagemath/sage/pull/36592/files. I will also try to take a look; I can certainly run tests, check the documentation, etc.

@kwankyu
Copy link
Collaborator Author

kwankyu commented Nov 19, 2023

Sorry for being late.

Sorry, I don't know how to review things on github. I think you'll have quicker results if someone else does it.

I thank John and Nils for taking care of this PR.

If you find some time, you can browse the changes at https://github.com/sagemath/sage/pull/36592/files. I will also try to take a look; I can certainly run tests, check the documentation, etc.

Reviewing PRs, especially mathematical ones, on github is relatively easy. Perhaps I can summarize the procedure as follows:

  1. Check if what the PR achieves as described in the PR description is worth to be merged to Sage. This needs your expertise on the subject domain.

  2. Check if the code changes does only what the PR intends to do. This can be done by reading the new doctests or checking the mere existence of them. It is also important to check the documentation changes. For this PR, find pull_from_function_field in the "diffsite" and the "diffsite".

  3. Check if the new code does not break Sage. This is certified if you see "All checks have passed" below.

  4. Optionally you can play with the PR to suggest further improvement to the author. Currently this requires that you build Sage on the PR branch on your own system. I think this is the most time-consuming part of the review procedure. But for a simple PR like the present one, you can skip this part.

If you need any assistance on any of these steps, I am here.

@jhpalmieri
Copy link
Member

Great, I am happy with it. Let's merge it!

Copy link

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

@kwankyu
Copy link
Collaborator Author

kwankyu commented Nov 30, 2023

Thank both of you, for pointing me to the opportunity to fill the feature gap!

vbraun pushed a commit to vbraun/sage that referenced this pull request Dec 4, 2023
    
<!-- ^^^^^
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 -->

For an algebraic curve C, `C.pull_from_function_field()` provides the
inverse map of `C.function()` between the fraction field of the
coordinate ring of C and the abstract function field of C.  This
tightens the integration of the two isomorphic fields. Elements of the
fraction field of the coordinate ring of C is represented by elements of
the fraction field of the coordinate ring of the ambient affine or
projective space, for user's convenience.

This is the missing feature as discussed in

https://groups.google.com/g/sage-support/c/Axkaex63f8w/m/wKL0TzmDAwAJ

The problem there can be solved by
```sage
sage: P2.<x,y,z> = ProjectiveSpace(QQ, 2)
sage: f = 2*x^5 - 4*x^3*y*z + x^2*y*z^2 + 2*x*y^3*z + 2*x*y^2*z^2+ y^5
sage: C = Curve(f)
sage: K = C.function(x/y).differential().divisor()  # canonical divisor
sage: basis = (-K).basis_function_space()
sage: Basis = [C.pull_from_function_field(f) for f in basis]
sage: phi = C.hom(Basis, P2)
sage: D = phi.image()  # conic
sage: D.degree()
2
sage: D
Closed subscheme of Projective Space of dimension 2 over Rational Field
defined by:
  x^2 + x*y + 2*y*z
```

<!-- 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.
- [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.

### ⌛ 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#36592
Reported by: Kwankyu Lee
Reviewer(s): John H. Palmieri, Kwankyu Lee
@vbraun vbraun merged commit 779a502 into sagemath:develop Dec 6, 2023
19 of 22 checks passed
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