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

Drinfeld modules: Make some imports lazy #35275

Merged
merged 3 commits into from
Apr 1, 2023

Conversation

mkoeppe
Copy link
Contributor

@mkoeppe mkoeppe commented Mar 13, 2023

📚 Description

We replace some imports by lazy_imports in sage.categories.drinfeld_modules and make the import of DrinfeldModule into the global namespace lazy. We also move the module-level import of PolynomialBaseringInjection in sage.rings.polynomial.polynomial_ring into a method.

git grep '^from sage.rings' src/sage/categories reveals that the recently added .drinfeld_modules is the only module that imports nontrivial things from sage.rings. This is a new obstacle to modularization (it caused an error due to import cycles in #35095.)

The purpose of reducing the module-level import to a method-level import is explained in https://doc.sagemath.org/html/en/developer/packaging_sage_library.html#module-level-runtime-dependencies

📝 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

Copy link
Contributor

@kryzar kryzar left a comment

Choose a reason for hiding this comment

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

Thank you @mkoeppe for taking the initiative of writing the code, and requesting my review. I approve the changes, with only one minor suggestion.

src/sage/categories/drinfeld_modules.py Outdated Show resolved Hide resolved
@kryzar kryzar self-requested a review March 14, 2023 20:53
Copy link
Contributor

@kryzar kryzar left a comment

Choose a reason for hiding this comment

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

Looks perfect, thank you for making the change.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Mar 14, 2023

Thanks for reviewing!

@github-actions
Copy link

Documentation preview for this PR is ready! 🎉
Built with commit: 72111c6

@codecov-commenter
Copy link

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.01 ⚠️

Comparison is base (f449b14) 88.62% compared to head (72111c6) 88.61%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #35275      +/-   ##
===========================================
- Coverage    88.62%   88.61%   -0.01%     
===========================================
  Files         2148     2148              
  Lines       398653   398655       +2     
===========================================
- Hits        353308   353274      -34     
- Misses       45345    45381      +36     
Impacted Files Coverage Δ
src/sage/categories/drinfeld_modules.py 94.33% <100.00%> (+0.05%) ⬆️
src/sage/rings/function_field/all.py 100.00% <100.00%> (ø)
src/sage/rings/polynomial/polynomial_ring.py 94.15% <100.00%> (-0.13%) ⬇️

... and 28 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.

@xcaruso xcaruso self-requested a review March 16, 2023 07:48
@vbraun vbraun merged commit 0f7314d 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 -->
We should be able to import Python modules from `sage.categories` even
if implementation modules are not installed; that's the premise of the
distribution **sagemath-categories**.
Here we make changes to avoid module-level dependencies on some higher-
level functionality, such as symbolic functions, linear algebra, etc.
<!-- 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" -->

#35275 takes care of some other imports.

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.
- [ ] 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: ...
-->
    
URL: #35279
Reported by: Matthias Köppe
Reviewer(s): Kwankyu Lee
@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.

4 participants