Opened 22 months ago
Closed 22 months ago
#27791 closed enhancement (fixed)
Py3: Minor enhancements in rings.polynomial.multi_polynomial.pyx
Reported by: | vklein | Owned by: | |
---|---|---|---|
Priority: | minor | Milestone: | sage-8.8 |
Component: | python3 | Keywords: | |
Cc: | Merged in: | ||
Authors: | Vincent Klein | Reviewers: | John Palmieri |
Report Upstream: | N/A | Work issues: | |
Branch: | c532b8d (Commits, GitHub, GitLab) | Commit: | c532b8de9357aebf4640cdca1db5d553c153d9c5 |
Dependencies: | Stopgaps: |
Description (last modified by )
- Replace a doctest with #random with a doctest returning the same result in py2 and py3.
- Remove unused imports.
Change History (12)
comment:1 Changed 22 months ago by
- Branch set to u/vklein/27791
comment:2 Changed 22 months ago by
- Commit set to 2e3024db27147dfad22bcc86cb666b41be033906
- Description modified (diff)
- Status changed from new to needs_review
comment:3 follow-up: ↓ 4 Changed 22 months ago by
I'm not sure it makes sense to sort list(ff)
. In principle, you could reconstruct the function from this list, but not if you sort it. In particular, once you sort, the previous comment "Currently, we use a fairly unoptimized method that evaluates one monomial at a time, with no sharing of repeated computations and with useless additions of 0 and multiplications by 1" is hard to make sense of.
comment:4 in reply to: ↑ 3 ; follow-up: ↓ 5 Changed 22 months ago by
Replying to jhpalmieri:
I'm not sure it makes sense to sort
list(ff)
. In principle, you could reconstruct the function from this list, but not if you sort it.
Does that mean even if the list comes in a different order in py2 and py3 you will get an equivalent function by reconstruction ?
If this is the case what do you think is the best fix ? #py2
#py3
tags.
comment:5 in reply to: ↑ 4 Changed 22 months ago by
Replying to vklein:
Replying to jhpalmieri:
I'm not sure it makes sense to sort
list(ff)
. In principle, you could reconstruct the function from this list, but not if you sort it.Does that mean even if the list comes in a different order in py2 and py3 you will get an equivalent function by reconstruction ?
This all guesswork on my part, but I think so. The list is just giving the summands in two different orders.
'push 0.0', 'push 12.0', 'load 1', 'load 2', 'dup', 'mul', 'mul', 'mul', 'add',
says to store 0, then store 12, take a factor of y ("load 1"), take a factor of z ("load 2"), duplicate it to get two factors of z, multiply (so you have z^2
), multiply again (so you have y z^2
), multiply again (to incorporate the factor of 12), then add to the 0 from the start.
With Python 3, it starts with the summand 9z^4
instead. The ordering in Python 3 seems to match the order in which the polynomial is actually printed, for what that's worth.
If this is the case what do you think is the best fix ?
#py2
#py3
tags.
How about a new doctest: instead of list(ff)
, do something like this after the comment:
sage: g = (x*y**2*z)._fast_float_() sage: list(g) [ insert output here ]
Since there is only one summand, there should be no issues with orderings, and I think it still illustrates the same issue.
comment:6 Changed 22 months ago by
- Commit changed from 2e3024db27147dfad22bcc86cb666b41be033906 to 9567cc99949d028dd410dba43afda8ffc0cac2cb
Branch pushed to git repo; I updated commit sha1. New commits:
9567cc9 | Trac #27791: Implement reviewer's comment.
|
comment:7 Changed 22 months ago by
It works ! Thanks for your help.
comment:8 Changed 22 months ago by
- Status changed from needs_review to needs_work
Merge failed with 8.8.beta5
comment:9 Changed 22 months ago by
- Commit changed from 9567cc99949d028dd410dba43afda8ffc0cac2cb to c532b8de9357aebf4640cdca1db5d553c153d9c5
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
c532b8d | Trac #27791: Fix multi_polynomial.pyx doctests ...
|
comment:10 Changed 22 months ago by
- Description modified (diff)
- Priority changed from major to minor
- Status changed from needs_work to needs_review
- Summary changed from Py3: Fix doctests in rings.polynomial.multi_polynomial.pyx to Py3: Minor enhancements in rings.polynomial.multi_polynomial.pyx
Rebased with 8.8.beta5 which already fix the py3 doctests.
comment:11 Changed 22 months ago by
- Reviewers set to John Palmieri
- Status changed from needs_review to positive_review
Looks good to me.
comment:12 Changed 22 months ago by
- Branch changed from u/vklein/27791 to c532b8de9357aebf4640cdca1db5d553c153d9c5
- Resolution set to fixed
- Status changed from positive_review to closed
New commits:
Trac #27791: Fix multi_polynomial.pyx doctests ...