cleaning and enhancement to PolyDict #34000 #35020
Conversation
Codecov ReportBase: 88.59% // Head: 88.59% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## develop #35020 +/- ##
===========================================
- Coverage 88.59% 88.59% -0.01%
===========================================
Files 2136 2136
Lines 396142 396156 +14
===========================================
+ Hits 350948 350958 +10
- Misses 45194 45198 +4
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. |
| -y + 1 | ||
| """ | ||
| return self.__class__(self.parent(), -self.__element) | ||
|
|
||
| def _add_(self, right): |
| return self.__class__(self.parent(),self.__element + right.__element) | ||
| elt = self.__element + right.__element | ||
| elt.remove_zeros() | ||
| return self.__class__(self.parent(), elt) | ||
|
|
||
| def _sub_(self, right): |
| S = P.change_ring(Q) | ||
|
|
||
| if P is not S: | ||
| return S.coerce(self).derivative(var) |
There was a problem hiding this comment.
Shouldn't this be S.coerce(self).integral(var)?
| pdict = new_pdict | ||
| if isinstance(pdict, list): | ||
| v = {} | ||
| L = <list> pdict |
There was a problem hiding this comment.
I don't think using <list> here does anything; perhaps just remove this line and update the iteration on the next.
There was a problem hiding this comment.
From what I thought this changes the behavior of Cython. The <list> cast is supposedly optimized assuming pdict is a list.
| pdict = new_pdict | ||
| if isinstance(pdict, list): | ||
| v = {} | ||
| L = <list> pdict |
There was a problem hiding this comment.
I don't think using <list> here does anything; perhaps just remove this line and update the iteration on the next.
| result._nonzero -= 1 | ||
| rindex -= 1 | ||
| need_to_add = 0 | ||
| result._data = <int*> sig_malloc(sizeof(int) * (self._nonzero + 1) * 2) |
There was a problem hiding this comment.
Is it a problem that self._nonzero + 1 might be wrong? I'm not sure what the __dealloc__ looks like, but this might cause a memory leak.
There was a problem hiding this comment.
A memory leak definitely not. But there could be an overflow in the operation sizeof(int) * (self._nonzero + 1) * 2. As this is not the only place where is it is not checked I would not worry too much.
| @@ -2241,8 +2438,7 @@ cdef class ETuple: | |||
|
|
|||
|
|
|||
| def make_PolyDict(data): | |||
There was a problem hiding this comment.
Actually make_PolyDict is useless (except for old pickles from previous sage versions).
| @@ -857,7 +857,7 @@ def groebner_basis_pote(I, prec, verbose=0): | |||
| sage: I.groebner_basis(algorithm="PoTe", prec=100) # indirect doctest | |||
| [...0000000001*x^3 + ...2222222222*y + ...000000000*x^2*y^2 + O(3^99 * <x, y>), | |||
| ...0000000001*x^2*y + ...01210121020 + O(3^100 * <x, y>), | |||
| ...0000000001*y^2 + ...01210121020*x + ...000000000*x^2*y^3 + ...0000000000*x^3*y + O(3^99 * <x, y>)] | |||
There was a problem hiding this comment.
Removing inexact zeros is a regression. I know that there was some discussion on the ticket (and also that handling inexact zeros overall in Sage isn't perfect), but perhaps the remove_zeros method can check to see if the base ring is inexact? Ideally I'd like to keep inexact zeros for most p-adic rings (excepting floating point and fixed mod precision), power series, and real interval fields.
There was a problem hiding this comment.
Inexactitude of the base ring is not accurate enough. For example, you do want to remove zeros for floating points.
|
The merge ddb15a3 made the comments outdated. It is very annoying to introduce merge commits without any serious reason. |
<!-- ^^^^^ 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 #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 <!-- 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. - [x] I have created tests covering the changes. - [x] I have updated the documentation accordingly. URL: #35174 Reported by: Vincent Delecroix Reviewer(s): FrΓ©dΓ©ric Chapoton, Travis Scrimshaw, Vincent Delecroix
|
This now conflicts with the changes done elsewhere. |
This fixes #34000
π Description
π Checklist
β Dependencies