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.

Sign up for GitHub

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

Jump to bottom

sage.rings.polynomial: Modularization fixes, # needs #36045

Merged
merged 58 commits into from
Aug 13, 2023
Merged

sage.rings.polynomial: Modularization fixes, # needs #36045

merged 58 commits into from
Aug 13, 2023

Conversation

mkoeppe
Copy link
Contributor

@mkoeppe mkoeppe commented Aug 7, 2023 •
edited
Loading

📝 Checklist

  • The title is concise, informative, and self-explanatory.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

⌛ Dependencies

Matthias Koeppe added 11 commits August 7, 2023 14:25
@mkoeppe
sage.rings.polynomial: Delay some imports
8530771
@mkoeppe
src/sage/rings/polynomial/polynomial_singular_interface.py: Do not fa…
6099c16 
…il if singular cannot be imported
@mkoeppe
pkgs/sagemath-categories: Add parts of sage.rings.polynomial
fb79635
@mkoeppe
src/sage/rings/polynomial/polynomial_element_generic.py: Use try...ex…
54d7976 
…cept for an unpickle_override import
@mkoeppe
src/sage/rings/polynomial/polynomial_element.pyx: Add missing imports…
bec037e 
… of RR
@mkoeppe
src/sage/rings/polynomial/polynomial_ring.py: Do not fail when Matrix…
5d2b0ce 
…Space cannot be imported
@mkoeppe
src/sage/rings/polynomial/multi_polynomial.pyx: Move import from sage…
37ac6f3 
….combinat into method
@mkoeppe
sage.rings.polynomial.multi_polynomial_{ideal,sequence}: Do not fail …
2cc264d 
…when sage.interfaces.singular is not available
@mkoeppe
sage.rings: Modularization fixes for imports of power series
cd5a852
@mkoeppe
src/sage/rings/polynomial/hilbert.pyx: Use sage.interfaces.abc
47bfef2
@mkoeppe
{Polynomial,MPolynomial_polydict}.{local,global}_height: Use sage.rin…
7248a07 
…gs.abc, fix handling of Orders
@mkoeppe mkoeppe self-assigned this Aug 7, 2023
@mkoeppe mkoeppe force-pushed the sage_rings_polynomial_modularization branch from 87f7b4a to f704277 Compare August 8, 2023 02:48
dcoudert
dcoudert suggested changes Aug 8, 2023
View reviewed changes
Copy link
Contributor

@dcoudert dcoudert left a comment

Choose a reason for hiding this comment

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

This is a giant ticket. Can't it be split per sub directories ?

There are several files in which you used # optional and not # needs. Also, block scope could be employed in many places. I noted only a few of these places (too many).

src/sage/rings/lazy_series.py Outdated Show resolved Hide resolved
src/sage/rings/lazy_series.py Outdated Show resolved Hide resolved
src/sage/rings/abc.pyx Outdated Show resolved Hide resolved
src/sage/rings/polynomial/laurent_polynomial_ring.py Outdated Show resolved Hide resolved
src/sage/rings/polynomial/multi_polynomial_element.py Outdated Show resolved Hide resolved
src/sage/rings/polynomial/polynomial_quotient_ring.py Outdated Show resolved Hide resolved
src/sage/rings/polynomial/polynomial_quotient_ring.py Outdated Show resolved Hide resolved
src/sage/rings/polynomial/polynomial_quotient_ring_element.py Outdated Show resolved Hide resolved
src/sage/rings/polynomial/polynomial_quotient_ring_element.py Outdated Show resolved Hide resolved
src/sage/rings/rational_field.py Outdated Show resolved Hide resolved
@mkoeppe mkoeppe force-pushed the sage_rings_polynomial_modularization branch 2 times, most recently from 557986c to 2b21016 Compare August 8, 2023 22:18
Matthias Koeppe added 13 commits August 8, 2023 15:22
@mkoeppe
src/sage/rings/polynomial/multi_polynomial_libsingular.pyx: Restore a…
8af9b87 
… '# random'
@mkoeppe
./sage -fixdoctests --distribution sagemath-categories --only-tags --…
6f03124 
…probe sage.rings.finite_rings src/sage/rings/polynomial
@mkoeppe
sage.rings.polynomial: Update # needs
8a1f33d
@mkoeppe
sage.rings.polynomial: Update # needs
c7698e5
@mkoeppe
sage.rings.{finite_rings,polynomial}: Modularization fixes for imports
b6a3f5a
@mkoeppe
sage.rings: Update # needs
33c6dd2
@mkoeppe
sage.rings.polynomial: Update # needs, use block tags
0040121
@mkoeppe
sage.rings.polynomial: Update # needs
c044016
@mkoeppe
sage.rings.polynomial: Update from sagemath#35095
6f14512
@mkoeppe
sage.rings: Fix circular import
c7625ab
@mkoeppe
sage.rings.polynomial: Update from sagemath#35095
266e4e0
@mkoeppe
sage.rings.polynomial: Many more block tags
f4a10a0
@mkoeppe
sage.rings.polynomial: More block tags, doctest cosmetics
ae81a48
@mkoeppe mkoeppe force-pushed the sage_rings_polynomial_modularization branch from 2b21016 to ae81a48 Compare August 8, 2023 22:22
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Aug 9, 2023

This is a giant ticket. Can't it be split per sub directories ?

There were a few commits with incidental changes in other directories. I have trimmed them away now.

There are several files in which you used # optional and not # needs. Also, block scope could be employed in many places. I noted only a few of these places (too many).

Thanks for your patient review of this megaticket. I've made the indicated changes and many more.

dcoudert
dcoudert suggested changes Aug 9, 2023
View reviewed changes
Copy link
Contributor

@dcoudert dcoudert left a comment

Choose a reason for hiding this comment

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

This is much better, but I noted a few more issues.

src/sage/rings/polynomial/ore_polynomial_element.pyx Outdated Show resolved Hide resolved
src/sage/rings/polynomial/ore_polynomial_element.pyx Outdated Show resolved Hide resolved
src/sage/rings/polynomial/polynomial_element.pyx Outdated Show resolved Hide resolved
src/sage/rings/polynomial/polynomial_element.pyx Outdated Show resolved Hide resolved
src/sage/rings/polynomial/polynomial_quotient_ring.py Outdated Show resolved Hide resolved
src/sage/rings/polynomial/polynomial_quotient_ring.py Outdated Show resolved Hide resolved
src/sage/rings/polynomial/term_order.py Outdated Show resolved Hide resolved
src/sage/rings/polynomial/laurent_polynomial_ring_base.py Outdated Show resolved Hide resolved
src/sage/rings/polynomial/multi_polynomial.pyx Outdated Show resolved Hide resolved
src/sage/rings/polynomial/multi_polynomial.pyx Outdated Show resolved Hide resolved
@github-actions
Copy link

github-actions bot commented Aug 9, 2023

Documentation preview for this PR (built with commit fafd930; changes) is ready! 🎉

dcoudert
dcoudert approved these changes Aug 10, 2023
View reviewed changes
Copy link
Contributor

@dcoudert dcoudert 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 Aug 10, 2023

Thank you!

vbraun pushed a commit to vbraun/sage that referenced this pull request Aug 11, 2023
sagemathgh-36045: sage.rings.polynomial: Modularization fixes, `# n…
fa95271 
…eeds`

    
<!-- ^^^^^
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 sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->

<!-- Why is this change required? What problem does it solve? -->
- Part of: sagemath#29705
- Cherry-picked from: sagemath#35095
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 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! -->
<!-- Feel free to remove irrelevant items. -->

- [x] The title is concise, informative, and self-explanatory.
- [ ] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36045
Reported by: Matthias Köppe
Reviewer(s): David Coudert
@vbraun vbraun merged commit ba7d118 into sagemath:develop Aug 13, 2023
9 of 12 checks passed
@mkoeppe mkoeppe added this to the sage-10.1 milestone Aug 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dcoudert dcoudert dcoudert approved these changes

Assignees

@mkoeppe mkoeppe

Labels
c: algebra t: refactoring
Projects
None yet
Milestone
sage-10.1
Development

Successfully merging this pull request may close these issues.
3 participants
@mkoeppe @dcoudert @vbraun