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

Py3: Minor enhancements in rings.polynomial.multi_polynomial.pyx #27791

Closed
vinklein mannequin opened this issue May 7, 2019 · 18 comments
Closed

Py3: Minor enhancements in rings.polynomial.multi_polynomial.pyx #27791

vinklein mannequin opened this issue May 7, 2019 · 18 comments

Comments

@vinklein
Copy link
Mannequin

vinklein mannequin commented May 7, 2019

  • Replace a doctest with #random with a doctest returning the same result in py2 and py3.
  • Remove unused imports.

Component: python3

Author: Vincent Klein

Branch/Commit: c532b8d

Reviewer: John Palmieri

Issue created by migration from https://trac.sagemath.org/ticket/27791

@vinklein vinklein mannequin added this to the sage-8.8 milestone May 7, 2019
@vinklein vinklein mannequin added c: python3 labels May 7, 2019
@vinklein
Copy link
Mannequin Author

vinklein mannequin commented May 7, 2019

Branch: u/vklein/27791

@vinklein
Copy link
Mannequin Author

vinklein mannequin commented May 7, 2019

New commits:

2e3024dTrac #27791: Fix multi_polynomial.pyx doctests ...

@vinklein

This comment has been minimized.

@vinklein
Copy link
Mannequin Author

vinklein mannequin commented May 7, 2019

Commit: 2e3024d

@vinklein vinklein mannequin added the s: needs review label May 7, 2019
@jhpalmieri
Copy link
Member

comment:3

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.

@vinklein
Copy link
Mannequin Author

vinklein mannequin commented May 9, 2019

comment:4

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.

@jhpalmieri
Copy link
Member

comment:5

Replying to @vinklein:

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 10, 2019

Changed commit from 2e3024d to 9567cc9

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 10, 2019

Branch pushed to git repo; I updated commit sha1. New commits:

9567cc9Trac #27791: Implement reviewer's comment.

@vinklein
Copy link
Mannequin Author

vinklein mannequin commented May 10, 2019

comment:7

It works ! Thanks for your help.

@vinklein
Copy link
Mannequin Author

vinklein mannequin commented May 13, 2019

comment:8

Merge failed with 8.8.beta5

@vinklein vinklein mannequin added s: needs work and removed s: needs review labels May 13, 2019
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 13, 2019

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

c532b8dTrac #27791: Fix multi_polynomial.pyx doctests ...

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 13, 2019

Changed commit from 9567cc9 to c532b8d

@vinklein
Copy link
Mannequin Author

vinklein mannequin commented May 13, 2019

comment:10

Rebased with 8.8.beta5 which already fix the py3 doctests.

@vinklein

This comment has been minimized.

@vinklein vinklein mannequin added s: needs review and removed s: needs work labels May 13, 2019
@vinklein vinklein mannequin changed the title Py3: Fix doctests in rings.polynomial.multi_polynomial.pyx Py3: Minor enhancements in rings.polynomial.multi_polynomial.pyx May 13, 2019
@vinklein vinklein mannequin added p: minor / 4 and removed p: major / 3 labels May 13, 2019
@jhpalmieri
Copy link
Member

comment:11

Looks good to me.

@jhpalmieri
Copy link
Member

Reviewer: John Palmieri

@vbraun
Copy link
Member

vbraun commented May 24, 2019

Changed branch from u/vklein/27791 to c532b8d

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants