Skip to content

Comments

Deprecate is_Algebra, is_CommutativeAlgebra#35253

Merged
vbraun merged 7 commits intosagemath:developfrom
mkoeppe:deprecate_is_Algebra
Apr 1, 2023
Merged

Deprecate is_Algebra, is_CommutativeAlgebra#35253
vbraun merged 7 commits intosagemath:developfrom
mkoeppe:deprecate_is_Algebra

Conversation

@mkoeppe
Copy link
Contributor

@mkoeppe mkoeppe commented Mar 9, 2023 •
edited
Loading

📚 Description

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

@codecov-commenter
Copy link

codecov-commenter commented Mar 10, 2023 •
edited
Loading

Codecov Report

Patch coverage: 95.98% and project coverage change: +0.01 🎉

Comparison is base (52a81cb) 88.57% compared to head (52c9bc1) 88.59%.

Additional details and impacted files 
@@             Coverage Diff             @@
##           develop   #35253      +/-   ##
===========================================
+ Coverage    88.57%   88.59%   +0.01%     
===========================================
  Files         2140     2140              
  Lines       397273   397433     +160     
===========================================
+ Hits        351891   352102     +211     
+ Misses       45382    45331      -51
Impacted Files Coverage Δ
src/sage/schemes/elliptic_curves/ell_generic.py 93.11% <66.66%> (-0.12%) ⬇️
src/sage/categories/algebra_ideals.py 82.60% <70.00%> (-6.29%) ⬇️
src/sage/categories/algebra_modules.py 90.90% <75.00%> (-9.10%) ⬇️
src/sage/categories/commutative_algebra_ideals.py 88.88% <77.77%> (-11.12%) ⬇️
src/sage/interfaces/tachyon.py 87.93% <90.00%> (+0.43%) ⬆️
src/sage/schemes/elliptic_curves/gal_reps.py 82.23% <90.00%> (+0.04%) ⬆️
src/sage/quadratic_forms/quadratic_form.py 90.26% <95.65%> (+0.16%) ⬆️
src/sage/algebras/algebra.py 77.77% <100.00%> (-22.23%) ⬇️
src/sage/modular/quasimodform/element.py 99.20% <100.00%> (+0.06%) ⬆️
src/sage/rings/commutative_algebra.py 100.00% <100.00%> (ø)
... and 45 more

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

@tscrim
Copy link
Collaborator

tscrim commented Mar 11, 2023

One currently needs to be careful about testing against CommutativeAlgebras(base_ring) as the category of A might be CommutativeAlgebras(base_ring.category()) (this was done to not create large numbers of equivalent categories over, say, GF(p)). There needs to be a better mechanism to test this, but the current change actually makes the test more strict than it was before. So code in the wild might fail.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Mar 12, 2023

Nice catch. I'll add a test for this

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Mar 12, 2023

Hm... Do you have an example? It looks to me to be a non-issue because

sage: CommutativeAlgebras(QQ).is_subcategory(CommutativeAlgebras(QQ.category()))
True

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Mar 12, 2023

Or rather, because:

sage: QQ['a'].category()
Join of Category of euclidean domains and Category of commutative algebras over (number fields and quotient fields and metric spaces) and Category of infinite sets
sage: QQ['a'] in CommutativeAlgebras(QQ)
True
sage: QQ['a'] in CommutativeAlgebras(QQ.category())
True

@tscrim
Copy link
Collaborator

tscrim commented Mar 12, 2023

I guess this was fixed at some point (or I am remembering a slightly different but similar issue I had seen before). That's good.

Although now we have another issue which is that we don't want to create a whole family of categories over GF(p). So we should just replace Algebras(base_ring) with Algebras(base_ring.category()) and similar.

@github-actions
Copy link

github-actions bot commented Mar 12, 2023

Documentation preview for this PR is ready! 🎉
Built with commit: 52c9bc1

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Mar 12, 2023

Thanks, this seems to work

tscrim
tscrim approved these changes Mar 12, 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.

LGTM.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Mar 12, 2023

Thank you!

@vbraun vbraun merged commit 93d9f61 into sagemath:develop Apr 1, 2023
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

Assignees

@mkoeppe mkoeppe

Labels

c: categories t: refactoring

Projects

None yet

Milestone

sage-10.0

Development

Successfully merging this pull request may close these issues.

4 participants

@mkoeppe @codecov-commenter @tscrim @vbraun