Commit
1 year ago
gh-36741: Doctest hide option: Better detection of hidden packages <!-- ^^^^^ 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 --> <!-- 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. --> This PR implements the suggestion made in https://github.com/sagemath/sage/pull/36696#issuecomment-1807627101. 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 https://github.com/sagemath/sage/pull/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')] ``` ### :memo: 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. ### :hourglass: 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: https://github.com/sagemath/sage/pull/36741 Reported by: Sebastian Oehms Reviewer(s): Matthias Köppe, Sebastian Oehms
Author
Release Manager
Loading