Skip to content

Comments

sage.{algebras,combinat,matroids}: Replace imports from sage.*.all for namespace packages#35090

Merged
vbraun merged 12 commits intosagemath:developfrom
mkoeppe:t/34946/sage__algebras_combinat_matroids___replace_imports_from_sage___all_for_namespace_packages
Mar 19, 2023
Merged

sage.{algebras,combinat,matroids}: Replace imports from sage.*.all for namespace packages#35090
vbraun merged 12 commits intosagemath:developfrom
mkoeppe:t/34946/sage__algebras_combinat_matroids___replace_imports_from_sage___all_for_namespace_packages

Conversation

@mkoeppe
Copy link
Contributor

@mkoeppe mkoeppe commented Feb 12, 2023

📚 Description

Fixes #34946

📝 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

@codecov-commenter
Copy link

codecov-commenter commented Feb 12, 2023 •
edited
Loading

Codecov Report

Base: 88.60% // Head: 88.58% // Decreases project coverage by -0.02% ⚠️

Coverage data is based on head (912e4c5) compared to base (8f5bbd2).
Patch coverage: 98.59% of modified lines in pull request are covered.

Additional details and impacted files 
@@             Coverage Diff             @@
##           develop   #35090      +/-   ##
===========================================
- Coverage    88.60%   88.58%   -0.02%     
===========================================
  Files         2140     2140              
  Lines       397273   397287      +14     
===========================================
- Hits        352004   351950      -54     
- Misses       45269    45337      +68
Impacted Files Coverage Δ
...age/algebras/hecke_algebras/ariki_koike_algebra.py 96.03% <0.00%> (ø)
src/sage/combinat/words/morphism.py 92.51% <0.00%> (ø)
...e/combinat/root_system/reflection_group_complex.py 23.03% <66.66%> (+0.14%) ⬆️
src/sage/algebras/affine_nil_temperley_lieb.py 98.55% <100.00%> (ø)
src/sage/algebras/cluster_algebra.py 96.69% <100.00%> (ø)
src/sage/algebras/free_algebra.py 96.16% <100.00%> (ø)
src/sage/algebras/free_algebra_quotient.py 98.63% <100.00%> (ø)
src/sage/algebras/group_algebra.py 94.11% <100.00%> (ø)
src/sage/algebras/iwahori_hecke_algebra.py 82.76% <100.00%> (+0.07%) ⬆️
src/sage/algebras/lie_algebras/bch.py 100.00% <100.00%> (ø)
... and 109 more

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 at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Matthias Koeppe added 2 commits February 12, 2023 16:39
@mkoeppe
Merge tag '10.0.beta0' into t/34946/sage__algebras_combinat_matroids_…
7bb289c 
…__replace_imports_from_sage___all_for_namespace_packages

SageMath version 10.0.beta0, Release Date: 2023-02-12
@mkoeppe
Merge branch 'develop' into t/34946/sage__algebras_combinat_matroids_…
254431c 
…__replace_imports_from_sage___all_for_namespace_packages
tscrim
tscrim requested changes Feb 14, 2023
View reviewed changes
Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

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

Two basically trivially things if you want to do them:

  1. The imports from sage.arith.misc import GCD as gcd are somewhat unnecessary (like the Q as QQ).
  2. The FiniteField as GF in the matroids, similarly. I am wondering if we should just replace the GF with FiniteField.

Matthias Koeppe added 3 commits February 14, 2023 11:21
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 14, 2023

  • The imports from sage.arith.misc import GCD as gcd are somewhat unnecessary (like the Q as QQ).

Thanks, indeed gcd is also provided by sage.arith.misc; so I've made this simplification.

  • The FiniteField as GF in the matroids, similarly. I am wondering if we should just replace the GF with FiniteField.

I'll not do this one here; importing it as GF seems like a convenient abbreviation

tscrim
tscrim approved these changes Feb 16, 2023
View reviewed changes
Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

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

Fair enough. LGTM.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 16, 2023

Thank you!

tobiasdiez
tobiasdiez suggested changes Feb 16, 2023
View reviewed changes
tobiasdiez
tobiasdiez reviewed Feb 18, 2023
View reviewed changes
src/sage/matroids/linear_matroid.pyx Outdated
Comment on lines 134 to 135
from sage.rings.finite_rings.finite_field_constructor import FiniteField
from sage.rings.finite_rings.finite_field_constructor import FiniteField as GF
Copy link
Contributor

@tobiasdiez tobiasdiez Feb 18, 2023

Choose a reason for hiding this comment

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

These imports seem to be duplicates

Copy link
Contributor Author

@mkoeppe mkoeppe Feb 18, 2023

Choose a reason for hiding this comment

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

Yes, and the first one was not even used. I've cleaned it up now

@mkoeppe
Merge remote-tracking branch 'upstream/develop' into t/34946/sage__al…
912e4c5 
…gebras_combinat_matroids___replace_imports_from_sage___all_for_namespace_packages
@github-actions
Copy link

github-actions bot commented Feb 24, 2023

Documentation preview for this PR is ready! 🎉
Built with commit: 912e4c5

@vbraun vbraun merged commit b0df140 into sagemath:develop Mar 19, 2023
vbraun pushed a commit that referenced this pull request Mar 26, 2023
80511e1 
gh-35110: Meta-PR: Replace imports from sage.*.all for namespace 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"
-->
### 📚 Description
Fixes #34201

<!-- 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" -->

### 📝 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 #35090
- Depends on #35098
- Depends on #35099
- Depends on #35105
- Depends on #35106
- Depends on #35107
    
URL: #35110
Reported by: Matthias Köppe
Reviewer(s): Dima Pasechnik
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tscrim tscrim tscrim approved these changes

+1 more reviewer

@tobiasdiez tobiasdiez tobiasdiez approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@mkoeppe mkoeppe

Labels

t: refactoring

Projects

None yet

Milestone

sage-10.0

Development

Successfully merging this pull request may close these issues.

sage.{algebras,combinat,matroids}: Replace imports from sage.*.all for namespace packages

6 participants

@mkoeppe @codecov-commenter @tscrim @tobiasdiez @vbraun @alexchandler100