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

cleaning and enhancement to PolyDict #35174

Merged
merged 47 commits into from
Mar 26, 2023
Merged

cleaning and enhancement to PolyDict #35174

merged 47 commits into from
Mar 26, 2023

Conversation

videlec
Copy link
Contributor

@videlec videlec commented Feb 22, 2023

📚 Description

Fixes #34000 (and more complete version of the proposition at #35020).

A discussion about the general problem of inexact zeros has been opened at #35319

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

@roed314
Copy link
Contributor

roed314 commented Feb 22, 2023

@videlec I'm currently in Chamonix with bad internet; I'll try to take a look at this over the weekend.

@codecov-commenter
Copy link

codecov-commenter commented Feb 23, 2023

Codecov Report

Patch coverage: 97.13% and project coverage change: +0.03 🎉

Comparison is base (e28bd1a) 88.58% compared to head (db9aed3) 88.61%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #35174      +/-   ##
===========================================
+ Coverage    88.58%   88.61%   +0.03%     
===========================================
  Files         2141     2148       +7     
  Lines       397475   398873    +1398     
===========================================
+ Hits        352106   353472    +1366     
- Misses       45369    45401      +32     
Impacted Files Coverage Δ
src/sage/schemes/elliptic_curves/ell_generic.py 93.25% <66.66%> (+0.01%) ⬆️
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.30% <90.90%> (+0.20%) ⬆️
.../sage/rings/polynomial/multi_polynomial_element.py 90.71% <94.00%> (-0.39%) ⬇️
src/sage/modular/quasimodform/element.py 99.20% <100.00%> (+0.06%) ⬆️
src/sage/rings/polynomial/multi_polynomial_ring.py 86.25% <100.00%> (+0.35%) ⬆️
src/sage/rings/qqbar.py 95.30% <100.00%> (+<0.01%) ⬆️
src/sage/schemes/affine/affine_morphism.py 90.33% <100.00%> (ø)
src/sage/schemes/elliptic_curves/BSD.py 43.75% <100.00%> (+0.21%) ⬆️
... and 42 more

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

@videlec
Copy link
Contributor Author

videlec commented Mar 19, 2023

The situation with trailing zeros evaluating to False is extremely annoying. For example equality of polynomials is not equality of coefficients

sage: from sage.rings.polynomial.polydict import PolyDict
sage: R.<t> = PowerSeriesRing(QQ)
sage: PolyDict({(1, 0): O(t)}) == PolyDict({})
False
sage: PolyDict({(1, 0): O(t)}) == PolyDict({(1, 0): O(t^2)})
True

Similarly, bool(p) is probably wrong. Of course the same question holds for matrices, etc.

@tscrim
Copy link
Collaborator

tscrim commented Mar 19, 2023

I will take a look at this tonight.

Indeed, the removal of zeros in general (not just trailing) definitely is useful at keeping this consistent and fast an should be done as much as possible.

src/sage/rings/polynomial/polydict.pyx Outdated Show resolved Hide resolved
src/sage/rings/polynomial/polydict.pyx Show resolved Hide resolved
src/sage/rings/polynomial/polydict.pyx Outdated Show resolved Hide resolved
@videlec videlec mentioned this pull request Mar 20, 2023
@github-actions
Copy link

Documentation preview for this PR is ready! 🎉
Built with commit: 1a7eeac

@videlec
Copy link
Contributor Author

videlec commented Mar 20, 2023

Actually changing bool(x) to mean x is not an exact zero would solve all issues here! But Julian does not seems convinced about it yet at #35319.

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.

Thank you. LGTM. We can always do more changes later should we find other improvements.

@vbraun vbraun merged commit 1e77797 into sagemath:develop Mar 26, 2023
@mkoeppe mkoeppe added this to the sage-10.0 milestone Mar 26, 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.

cleaning and enhancement to PolyDict
8 participants