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 style guide / reference for # optional - sage.... doctest tags, extend sage -t and sage -fixdoctests for modularization tasks #35749

Merged
merged 157 commits into from
Jul 30, 2023

Conversation

mkoeppe
Copy link
Member

@mkoeppe mkoeppe commented Jun 9, 2023

📚 Description

Motivated by the sage-devel thread https://groups.google.com/g/sage-devel/c/utA0N1it0Eo (2023-06)

📝 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

@mkoeppe mkoeppe self-assigned this Jun 9, 2023
@mkoeppe mkoeppe changed the title Expand documentation on # optional - sagemath-... (modularization) Add style guide and reference for # optional - sagemath-... doctest annotations (for modularization) Jun 9, 2023
@mkoeppe mkoeppe mentioned this pull request Jun 9, 2023
5 tasks
@mkoeppe mkoeppe changed the title Add style guide and reference for # optional - sagemath-... doctest annotations (for modularization) Add style guide and reference for # optional - sage.... doctest annotations (for modularization) Jun 16, 2023
@mkoeppe
Copy link
Member Author

mkoeppe commented Jun 16, 2023

(the doctest failure in ell_rational_field is unrelated)

@mkoeppe
Copy link
Member Author

mkoeppe commented Jul 14, 2023

Thank you!

@vbraun
Copy link
Member

vbraun commented Jul 19, 2023

Doctests fail because now the previously-skipped broken files are picked up

sage -t --long --random-seed=286735480429121101562228604801325644303 sage/schemes/elliptic_curves/heegner.py  # 4 doctests failed
sage -t --long --random-seed=286735480429121101562228604801325644303 sage/schemes/elliptic_curves/hom_frobenius.py  # 2 doctests failed

@kwankyu
Copy link
Collaborator

kwankyu commented Jul 19, 2023

From sage-release

That's already fixed in #35749, which is why that PR is marked critical.

We didn't fix the failures. Just that @mkoeppe mentioned that they are not related with this PR. According to @vbraun, indeed they are.

@kwankyu
Copy link
Collaborator

kwankyu commented Jul 19, 2023

They all look easy to fix. All the doctests rely on arbitrary choice between $P$ and $-P$ which are points of an elliptic curve whose $x$-coordinates are the same. I am 100% sure about the doctests in age/schemes/elliptic_curves/hom_frobenius.py and 80% sure about the doctests in sage/schemes/elliptic_curves/heegner.py. Needs a look from an expert @yyyyx4.

@kwankyu
Copy link
Collaborator

kwankyu commented Jul 19, 2023

We may fix them in the style

--- a/src/sage/schemes/elliptic_curves/heegner.py
+++ b/src/sage/schemes/elliptic_curves/heegner.py
@@ -47,8 +47,8 @@ We find some Mordell-Weil generators in the rank 1 case using Heegner points::
     sage: E = EllipticCurve('43a'); P = E.heegner_point(-7)
     sage: P.x_poly_exact()
     x
-    sage: P.point_exact()
-    (0 : 0 : 1)
+    sage: p = P.point_exact(); p == E(0,0,1) or -p == E(0,0,1)
+    True
 
     sage: E = EllipticCurve('997a')
     sage: E.rank()

@kwankyu
Copy link
Collaborator

kwankyu commented Jul 19, 2023

Please merge #35966 here for test.

@github-actions
Copy link

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

vbraun pushed a commit that referenced this pull request Jul 20, 2023
… imports

    
<!-- Please provide a concise, informative and self-explanatory title.
-->
<!-- Don't put issue numbers in the title. Put it in the Description
below. -->
<!-- For example, instead of "Fixes #12345", use "Add a new method to
multiply two integers" -->

### 📚 Description

<!-- Describe your changes here in detail. -->
- Many `# optional` are removed because small prime finite fields no
longer need to be marked `# optional/needs sage.rings.finite_rings`
since #35847
- Other `# optional` are replaced by codeblock-scoped `# needs` tags
- Remaining `# optional` with standard features are replaced by `#
needs`
- Some import fixes needed for separate testing of **sagemath-modules**
or **sagemath-graphs**[modules] from #35095
<!-- 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 #12345". -->
This is:
- done with the help of `sage -fixdoctests` from #35749
- Part of: #29705
- Cherry-picked from: #35095
<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. It should be `[x]` not `[x
]`. -->

- [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.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

### ⌛ Dependencies

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

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: #35919
Reported by: Matthias Köppe
Reviewer(s): David Coudert, Matthias Köppe
vbraun pushed a commit that referenced this pull request Jul 20, 2023
…s, update `# needs`

    
<!-- ^^^^^
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 #1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->

- `# optional` with standard features are replaced by `# needs`
- Some import fixes needed for separate testing of **sagemath-graphs**
from #35095
<!-- 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 #12345". -->
This is:
- done with the help of `sage -fixdoctests` from #35749
- Part of: #29705
- Cherry-picked from: #35095

### 📝 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.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

### ⌛ Dependencies

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

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: #35951
Reported by: Matthias Köppe
Reviewer(s): David Coudert
@vbraun vbraun merged commit f2abd5a into sagemath:develop Jul 30, 2023
10 checks passed
vbraun pushed a commit that referenced this pull request Jul 30, 2023
…eds`

    
<!-- ^^^^^
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 #1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->

- Many `# optional` are removed because small prime finite fields no
longer need to be marked `# optional/needs sage.rings.finite_rings`
since #35847
- Other `# optional` are replaced by codeblock-scoped `# needs` tags
- Remaining `# optional` with standard features are replaced by `#
needs`
- Some import fixes needed for separate testing of **sagemath-graphs**
from #35095
<!-- 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 #12345". -->
This is:
- done with the help of `sage -fixdoctests` from #35749
- Part of: #29705
- Cherry-picked from: #35095

<!-- 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 #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.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

### ⌛ Dependencies

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

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: #35943
Reported by: Matthias Köppe
Reviewer(s): David Coudert, Matthias Köppe
vbraun pushed a commit that referenced this pull request Jul 30, 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 #1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->
- Many `# optional` are removed because small prime finite fields no
longer need to be marked `# optional/needs sage.rings.finite_rings`
since #35847
- Other `# optional` are replaced by codeblock-scoped `# needs` tags
- Remaining `# optional` with standard features are replaced by `#
needs`
- Some import fixes needed for separate testing of **sagemath-graphs**
from #35095
<!-- 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 #12345". -->
This is:
- done with the help of `sage -fixdoctests` from #35749
- Part of: #29705
- Cherry-picked from: #35095

### 📝 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.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

### ⌛ Dependencies

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

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: #35957
Reported by: Matthias Köppe
Reviewer(s): David Coudert, Matthias Köppe
@mkoeppe mkoeppe deleted the feature_definitions branch July 30, 2023 17:24
@mkoeppe mkoeppe added this to the sage-10.1 milestone Jul 30, 2023
mkoeppe added a commit to mkoeppe/sage that referenced this pull request Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants