Skip to content

Conversation

mkoeppe
Copy link
Contributor

@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
Copy link
Contributor Author

mkoeppe commented Jun 16, 2023

(the doctest failure in ell_rational_field is unrelated)

vbraun pushed a commit that referenced this pull request Jun 21, 2023
gh-35734: Reference manual: Show modularized sagelib packages separately
    
<!-- 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. -->
Packages such as **sagemath-categories** are currently mixed with
optional SPKGs in the reference guide.
We separate them out as a separate category.

We also move the section on `Feature`s right next to the package list.
#35749 and follow-ups will interlink these sections.

Preview:
- https://deploy-preview-35734--sagemath-
tobias.netlify.app/reference/spkg/index.html#packages-of-the-
modularized-sage-library
- https://deploy-preview-35734--sagemath-
tobias.netlify.app/reference/spkg/index.html#runtime-detectable-
features-and-conditional-doctests

<!-- 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". -->
- Part of #29705
<!-- 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.
- [ ] 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: #35734
Reported by: Matthias Köppe
Reviewer(s): Kwankyu Lee, Matthias Köppe
@kwankyu
Copy link
Collaborator

kwankyu commented Jul 12, 2023

I get

$ sage --fixdoctests --help
usage: sage-fixdoctests [-h] [-l] [--distribution DISTRIBUTION] [--venv VENV] [--environment ENVIRONMENT] [--no-test] [--full-tracebacks] [--only-tags]
                        [--probe FEATURES] [--keep-both] [--overwrite] [--no-overwrite]
                        [filename ...]

Given an input file with doctests, this creates a modified file that passes the doctests (modulo any raised exceptions). By default, the input file is
modified. You can also name an output file.

positional arguments:
  filename              input filenames; or INPUT_FILENAME OUTPUT_FILENAME if exactly two filenames are given and neither --overwrite nor --no-overwrite is
                        present

options:
  -h, --help            show this help message and exit
  -l, --long
  --distribution DISTRIBUTION
                        distribution package to test, e.g., 'sagemath-graphs', 'sagemath-combinat[modules]'; sets defaults for --venv and --environment
  --venv VENV           directory name of a venv where 'sage -t' is to be run
  --environment ENVIRONMENT
                        name of a module that provides the global environment for tests; implies --keep-both and --full-tracebacks
  --no-test             do not run the doctester, only rewrite '# optional/needs' tags; implies --only-tags
  --full-tracebacks     include full tracebacks rather than '...'
  --only-tags           only add '# optional/needs' tags where needed, ignore other failures
  --probe FEATURES      check whether '# optional - FEATURES' tags are still needed, remove these
  --keep-both           do not replace test results; duplicate the test instead and mark both copies # optional
  --overwrite           never interpret a second filename as OUTPUT; overwrite the source files
  --no-overwrite        never interpret a second filename as OUTPUT; output goes to files named INPUT.fixed

You may want to update the "positional arguments:" section.


sage: print_with_ruler([
....: update_optional_tags(' sage: unforced() # opt' 'ional - latte_int'),
....: update_optional_tags(' sage: unforced() # opt' 'ional - latte_int',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Splitting the string containing # optional is still necessary? The doctester or the fixer is not robust enough yet to handle the case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is tox -e relint that is not smart enough

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. OK.

Copy link
Collaborator

@kwankyu kwankyu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks very good to me as far as I can see.

@kwankyu
Copy link
Collaborator

kwankyu commented Jul 12, 2023

As this PR is motivated by the sage-devel discussion started by @tscrim, I invite him to review this PR once more.

If there's no comment from him for a couple of days, I will set it positive review.

@mkoeppe
Copy link
Contributor 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
gh-35919: `sage.matroids`: Update `# needs`, modularization fixes for 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
gh-35951: `sage.combinat.cluster_algebra_quiver`: Modularization fixes, 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
vbraun pushed a commit that referenced this pull request Jul 30, 2023
gh-35943: `sage.combinat.designs`: Modularization fixes, 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 -->

- 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
gh-35957: `sage.rings.function_field`: 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 -->
- 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
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.

Doctest # optional / # long / # needs annotation maintenance tool Codeblock-scoped # optional annotations for doctests (modularization)
4 participants