sage.features: Declare features as "standard" explicitly
#35820
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… into hide_features_34185
… into hide_features_34185
… into hide_features_34185
… into hide_features_34185
… into hide_features_34185
…rac.sagemath.org:sage into hide_features_34185
… into hide_features_34185
…emath/sagetrac-mirror into hide_features_34185
… type if SAGE_ROOT/build/pkgs is available
|
Looks promising. I am not even objecting to the term "butchered" :P even if I would prefer "annihilated". In any case, I should plan work on my own follow up. I think with that and some other stuff, getting rid of |
|
I think features for some optional packages are still missing; there may be some open tickets for some of these. When that is fixed, I'm all in favor of eliminating the use of sage.misc.packages from the doctester! |
|
Great! Thank you for promptly resolving this issue. It seems like a good solution. In the logs of the doctests there are some features with missing |
mkoeppe
force-pushed
the
feature_standard_optional_static
branch
from
b831dbf
to
ad202ba
Compare
|
Documentation preview for this PR (built with commit 4eddb7c; changes) is ready! 🎉 |
|
LGTM! |
soehms
approved these changes
Jun 25, 2023
|
Thanks for reviewing! |
32 tasks
I've opened #35856 to track the progress on that. |
vbraun
pushed a commit
that referenced
this pull request
Jul 30, 2023
…test tags, extend `sage -t` and `sage -fixdoctests` for modularization tasks
<!-- 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. -->
- Fixing the handling of file-level `# optional` tags.
- Some files were not being doctested; fixing the recently introduced
errors in doctests.
- Implementing block-scoped tags (originally done in PR #35779, merged
here)
- Expanding the documentation on the `# optional` / `# needs` tags used
for modularization purposes, with examples.
- Adding features `sage.modular`, `sage.numerical.mip`,
`sage.rings.complex_double`, `sage.sat`
- The tools `sage -t` and `sage --fixdoctests` receive some new options
for modularization tasks (see added documentation):
```
$ make pypi-wheels
$ make SAGE_CHECK=yes sagemath-modules
$ ./sage --fixdoctests --distribution sagemath-modules \
src/sage/combinat/root_system/root_lattice_realization_algebras.py
```
(example uses #35095)
- Suppressing `# optional`/`# needs` of present features in the help
system
.
<!-- Why is this change required? What problem does it solve? -->
Motivated by the sage-devel thread https://groups.google.com/g/sage-
devel/c/utA0N1it0Eo (2023-06)
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes #12345". -->
- Resolves #35790
- Resolves #35750
- 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.
- [x] I have linked a relevant issue or discussion.
- [ ] 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
- #12345: short description why this is a dependency
- #34567: ...
-->
- Depends on #35820 (merged here)
- Vote at https://groups.google.com/g/sage-devel/c/8KZNRBs6F6U (result:
https://groups.google.com/g/sage-devel/c/MtS2u3VbJEo)
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
URL: #35749
Reported by: Matthias Köppe
Reviewer(s): Kwankyu Lee, Matthias Köppe
5 tasks
vbraun
pushed a commit
to vbraun/sage
that referenced
this pull request
Mar 24, 2024
…kages
<!-- ^^^^^
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 -->
<!-- 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. -->
This PR implements the suggestion made in
sagemath#36696 (comment).
This means that it introduces a counter in the feature class to detect
the number of events a feature has been asked to be present while it was
hidden. This allows to remove the call of the `is_present` method in
`doctest/control.py`.
In order to test it I ran `sage -tp --all --hide=all`. Ideally this
should give *All tests passed*. But I got two failing files. One of
those was `src/sage/features/databases.py` because
`database_cremona_mini_ellcurve` was detected to be optional even
thought it is a standard package. This is a leftover of
sagemath#35820 which I fix here.
Similarily I got 37 failures in
`src/sage/groups/abelian_gps/abelian_group.py` since
`gap_package_polycyclic` was classified optional even though it is a Gap
standard package since Gap 4.10 (as well as `gap_package_atlasrep`). But
in this case a corresponding fix, i.e.:
```
def all_features():
- return [GapPackage("atlasrep", spkg="gap_packages"),
+ return [GapPackage("atlasrep", spkg="gap_packages",
type='standard'),
GapPackage("design", spkg="gap_packages"),
GapPackage("grape", spkg="gap_packages"),
GapPackage("guava", spkg="gap_packages"),
GapPackage("hap", spkg="gap_packages"),
- GapPackage("polycyclic", spkg="gap_packages"),
+ GapPackage("polycyclic", spkg="gap_packages",
type='standard'),
GapPackage("qpa", spkg="gap_packages"),
GapPackage("quagroup", spkg="gap_packages")]
```
is not the correct way (it gives `UserWarning: Feature
gap_package_polycyclic is declared standard, but it is provided by
gap_packages, which is declared optional in SAGE_ROOT/build/pkgs`). I
would say, the correct fix would be to remove both Gap packages from the
`all_features` list and erase the corresponding *optional tags* in
`permgroup_named.py`, `distance_regular.pyx`, `abelian_aut.py`,
`abelian_group.py` and `abelian_group_gap.py`. If you agree I will open
another PR for this.
BTW: there seem to be more packages with ambiguous type (detected with
current Docker image):
```
Digest:
sha256:790197bb223bd7e20b0a2e055aa7b4c5846dc86d94b2425cd233cb6160a5b944
Status: Downloaded newer image for sagemath/sagemath:develop
┌────────────────────────────────────────────────────────────────────┐
│ SageMath version 10.2.rc4, Release Date: 2023-11-17 │
│ Using Python 3.11.1. Type "help()" for help. │
└────────────────────────────────────────────────────────────────────┘
┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓
┃ Warning: this is a prerelease version, and it may be unstable. ┃
┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛
sage: from sage.features.all import all_features
sage: [(f, f._spkg_type()) for f in all_features() if f.is_present() and
f._spkg_type() != 'standard']
[(Feature('sage.misc.cython'), 'optional'),
(Feature('database_cremona_mini_ellcurve': Cremona's database of
elliptic curves),
'optional'),
(Feature('gap_package_atlasrep'), 'optional'),
(Feature('gap_package_polycyclic'), 'optional'),
(Feature('sage.libs.ecl'), 'optional'),
(Feature('sage.libs.gap'), 'optional'),
(Feature('sage.libs.singular'), 'optional'),
(Feature('sage.numerical.mip'), 'optional')]
sage:
sage: [(f, f._spkg_type()) for f in all_features() if not f.is_present()
and f._spkg_type() == 'standard']
[(Feature('sagemath_doc_html'), 'standard'), (Feature('sage.sat'),
'standard')]
```
### 📝 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#36741
Reported by: Sebastian Oehms
Reviewer(s): Matthias Köppe, Sebastian Oehms
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
📚 Description
As discussed in #35668 (comment)
📝 Checklist
⌛ Dependencies