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

sage.rings.polynomial.polynomial_ring[_constructor]: Handle missing implementation modules #35277

Merged
merged 6 commits into from
Apr 1, 2023

Conversation

mkoeppe
Copy link
Member

@mkoeppe mkoeppe commented Mar 13, 2023

๐Ÿ“š Description

We modify the __init__ methods of univariate polynomial implementation classes so that they all take an implementation keyword (and pass it on to their super).
We guard the imports of implementation classes in the PolynomialRing constructor with try... except ImportError and fall back to alternative implementations unless a specific implementation was requested.

This is part of:

๐Ÿ“ Checklist

  • I have made sure that the title is self-explanatory and the description concisely explains the PR.
  • I have linked an issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

โŒ› Dependencies

@github-actions
Copy link

Documentation preview for this PR is ready! ๐ŸŽ‰
Built with commit: 58712e2

@codecov-commenter
Copy link

Codecov Report

Patch coverage: 72.26% and project coverage change: -0.02 โš ๏ธ

Comparison is base (f449b14) 88.62% compared to head (58712e2) 88.61%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #35277      +/-   ##
===========================================
- Coverage    88.62%   88.61%   -0.02%     
===========================================
  Files         2148     2148              
  Lines       398653   398715      +62     
===========================================
+ Hits        353308   353314       +6     
- Misses       45345    45401      +56     
Impacted Files Coverage ฮ”
src/sage/rings/polynomial/polynomial_ring.py 90.39% <71.85%> (-3.88%) โฌ‡๏ธ
...ge/rings/polynomial/polynomial_ring_constructor.py 98.23% <100.00%> (-0.02%) โฌ‡๏ธ

... and 25 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

โ˜” View full report in Codecov by Sentry.
๐Ÿ“ข Do you have feedback about the report comment? Let us know in this issue.

@mezzarobba
Copy link
Member

We guard the imports of implementation classes in the PolynomialRing constructor with try... except ImportError and fall back to alternative implementations unless a specific implementation was requested.

Can you say a bit more about why you want to do that? I suppose it makes sense, but I am a bit concerned about PolynomialRing(ZZ) meaning different things depending on what is installed on your system. As far as I know the implementations are not perfectly interchangeable, and non-default combinations are largely untested.

Maybe we could issue a warning when the default implementation for some base ring cannot be used due to a failing import. Would that be too intrusive?

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 14, 2023

Can you say a bit more about why you want to do that? I suppose it makes sense, but I am a bit concerned about PolynomialRing(ZZ) meaning different things depending on what is installed on your system. As far as I know the implementations are not perfectly interchangeable, and non-default combinations are largely untested.

The main purpose of the modularization is to enable users/developers to use small parts of Sage that do not depend on many external libraries.
For example, in #35095 I am preparing a distribution sagemath-polyhedra, which uses polynomials only in a very basic way that doesn't need a specific implementation. These distributions also run doctests; currently a subset of the normal doctests is run (using the # optional annotations), but additional tests can be added for more test coverage.

Maybe we could issue a warning when the default implementation for some base ring cannot be used due to a failing import. Would that be too intrusive?

I don't think we need a run-time warning. The existing doctests already make sure that in a full installation of Sage, everything is in order.

@mezzarobba
Copy link
Member

Ok, thanks. I suppose we can add warnings later if it does prove too confusing.

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 15, 2023

Thank you!

@vbraun vbraun merged commit 9156b7b into sagemath:develop Apr 1, 2023
vbraun pushed a commit that referenced this pull request Apr 1, 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"
-->
### ๐Ÿ“š Description

<!-- Describe your changes here in detail -->
<!-- Why is this change required? What problem does it solve? -->
<!-- If it resolves an open issue, please link to the issue here. For
example "Closes #1337" -->

Using the ABC `sage.rings.number_field.number_field_base.NumberField`

Deprecating `is_NumberField`.

Part of:
- #29705

### ๐Ÿ“ 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! -->

- [x] I have made sure that the title is self-explanatory and the
description concisely explains the PR.
- [x] I have linked an issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

### โŒ› Dependencies
<!-- List all open pull requests that this PR logically depends on -->
<!--
- #xyz: short description why this is a dependency
- #abc: ...
-->
- Depends on #35277
    
URL: #35283
Reported by: Matthias Kรถppe
Reviewer(s): Marc Mezzarobba
@mkoeppe mkoeppe added this to the sage-10.0 milestone Apr 1, 2023
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.

None yet

4 participants