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

using "change_ring" in quadratic_forms #35248

Merged
merged 4 commits into from Apr 1, 2023

Conversation

fchapoton
Copy link
Contributor

📚 Description

Deprecate the non-standard base_change_to in favor of the standard change_ring inside quadratic forms.

📝 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

@fchapoton
Copy link
Contributor Author

We should also get rid (later) of base_change in src/sage/structure/factorization.py

@codecov-commenter
Copy link

codecov-commenter commented Mar 8, 2023

Codecov Report

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

Comparison is base (5330147) 88.61% compared to head (39fe637) 88.59%.

❗ Current head 39fe637 differs from pull request most recent head a6cd790. Consider uploading reports for the commit a6cd790 to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #35248      +/-   ##
===========================================
- Coverage    88.61%   88.59%   -0.02%     
===========================================
  Files         2148     2140       -8     
  Lines       398855   397417    -1438     
===========================================
- Hits        353438   352107    -1331     
+ Misses       45417    45310     -107     
Impacted Files Coverage Δ
src/sage/algebras/clifford_algebra.py 94.03% <100.00%> (-0.18%) ⬇️
src/sage/quadratic_forms/quadratic_form.py 90.30% <100.00%> (ø)
...ic_forms/quadratic_form__local_field_invariants.py 95.89% <100.00%> (ø)
...ic_forms/quadratic_form__mass__Siegel_densities.py 87.97% <100.00%> (ø)

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

@github-actions
Copy link

github-actions bot commented Mar 8, 2023

Documentation preview for this PR is ready! 🎉
Built with commit: 70e0303

Copy link
Member

@mkoeppe mkoeppe left a comment

Choose a reason for hiding this comment

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

LGTM

@vbraun
Copy link
Member

vbraun commented Mar 26, 2023

merge conflict

@vbraun vbraun merged commit 8aa0254 into sagemath:develop Apr 1, 2023
6 of 7 checks passed
@mkoeppe mkoeppe added this to the sage-10.0 milestone Apr 1, 2023
vbraun pushed a commit that referenced this pull request Apr 23, 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
We make `sage.quadratic_forms` ready for modularized distributions by:
- moving imports from some packages, such as
`sage.rings.padics.factory`, into methods
- using `lazy_import` to provide the methods of `QuadraticForm`
implemented in separate modules
- adding `# optional` annotations to doctests

Also some improvements to the code style of doctests and the Sphinx
markup of some docstrings have been made.

This has been and can be tested by merging into:
- #35095
**sagemath-polyhedra** ships vector spaces etc. and now also portions of
`sage.quadratic_forms` – see #35095
/files#diff-
f4c7c1beea87ae44c426de0a55459daef63b75b701b72bc136d88f4e7c9285cfR104

Resolves #35243

Part of:
- #29705

<!-- 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 #35248
- Depends on #35166
    
URL: #35305
Reported by: Matthias Köppe
Reviewer(s): Kwankyu Lee, Matthias Köppe
@fchapoton fchapoton deleted the change_ring_quadform branch July 16, 2023 19:25
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