Opened 13 years ago
Closed 11 years ago
#8718 closed enhancement (fixed)
Polynomial.apply_map()
Reported by: | Marc Mezzarobba | Owned by: | Alex Ghitza |
---|---|---|---|
Priority: | minor | Milestone: | sage-5.0 |
Component: | algebra | Keywords: | polynomial, map |
Cc: | Daniel Krenn | Merged in: | sage-5.0.beta6 |
Authors: | Marc Mezzarobba, Julian Rueth | Reviewers: | Daniel Krenn |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | #11981 | Stopgaps: |
Description (last modified by )
Computing, for instance, the complex conjugate of a polynomial currently requires going through its list or dictionary representation. Polynomials could provide a method similar to Matrix.apply_map() to make this kind of computations easier.
Apply
to the sage repository.
Attachments (4)
Change History (23)
Changed 13 years ago by
Attachment: | trac_8718_map_over_polynomial.patch added |
---|
comment:1 Changed 13 years ago by
Status: | new → needs_review |
---|
comment:2 Changed 13 years ago by
One suggestion---the names of the new functions do not seem to indicate to me that they only operate on the nonzero coefficients, so it would be hard for me to remember what the difference between map_coefficients and apply_map is.
How about just adding an argument to apply_map:
p.apply_map(nonzero_only=True)
or something like that?
comment:3 Changed 13 years ago by
The idea was to stay compatible with MPolynomial.map_coefficients()
, which operates on nonzero coefficients. Perhaps we could remove
apply_map()
rather that
map_coefficients()
, and add an option to make
map_coefficients()
map over zero coefficients too. (Ignoring them by default seems sensible, since (i) there is really no mathematical difference between zeros below and above the leading coefficient, and (ii) the functions one will typically pass to
map_coefficients()
are ring homomorphisms.)
What do you think?
comment:4 Changed 13 years ago by
That sounds cleaner to me. I'm likely not to use this function very much, so I shouldn't be the last authority on it, though.
comment:5 Changed 12 years ago by
Status: | needs_review → needs_work |
---|
Any progress here? I agree with Jason's contention that having both apply_map
and map_coefficients
is less than ideal, so I suggest that we put this back to "needs work" pending a patch that implements this.
comment:6 Changed 11 years ago by
I would suggest to map only non-zero coefficients because of the following reason: When mapping all (really all) zeros to a non-zero, the result is not a polynomial any more, so one would have to restrict that. But how should this restriction look like? A good restriction strategy (strategy which coefficients to choose and apply the map on) should be generalizable to the multivariate case. E.g. restriction by degree (i.e. only change the coefficients with indizes 0 to degree) could work in the univariate case, but does not work for multivariate polynomials.
If one really wants to map 0 to a non-zero, then this should be done somewhere else, i.e., not in map_coefficients
comment:7 Changed 11 years ago by
Authors: | → Marc Mezzarobba, Julian Rueth |
---|---|
Milestone: | sage-wishlist → sage-5.0 |
Status: | needs_work → needs_review |
I agree that mapping 0 to non-zero should not be a part of map_coefficients().
I rewrote the patch to only contain map_coefficients() for polynomial elements.
Was there any reason for distinguishing between polynomial_element and polynomial_element_generic? I removed the distinction and it turned out that this implementation is even faster for the example polynomials.
comment:9 follow-up: 10 Changed 11 years ago by
Thanks for taking care of that! (And shame on me that I didn't!)
As for the distinction between polynomial_element and polynomial_element_generic, it may indeed have been a speed issue, but I can't remember the details. Anyway, if the current version does the job, I'm fine with it.
[I'm not sure what the rules are here: am I allowed to review the new version of the patch though being its initial author?]
comment:10 Changed 11 years ago by
[I'm not sure what the rules are here: am I allowed to review the new version of the patch though being its initial author?]
Yes, in my experience this is ok and actually happens frequently.
comment:11 Changed 11 years ago by
Status: | needs_review → needs_work |
---|
In #11981 the map_coefficients of a multivariate polynomial was adapted to change the base ring of the coefficients. I think we should also do this here for this function, since the two should have the same behavior.
comment:12 follow-up: 13 Changed 11 years ago by
Status: | needs_work → needs_info |
---|
True. I wanted to push this to a later ticket but we can also talk about this here.
I wonder if this "new base ring" should default to the codomain of f (if the function is actually a sage 'Map'). For me this has always been what I wanted. Any opinions on that?
comment:13 Changed 11 years ago by
Work issues: | → check if function knows its codomain and use it |
---|
Replying to saraedum:
I wonder if this "new base ring" should default to the codomain of f (if the function is actually a sage 'Map'). For me this has always been what I wanted. Any opinions on that?
That is a good idea, we should implement that. So the strategy would be the following: If a new base ring is given, then we use it, otherwise, we check if the function knowns its codomain and if yes use it, otherwise we do not change the base ring.
comment:14 Changed 11 years ago by
Ok. I already have that implemented. Just need to add some more doctests and upload it.
Changed 11 years ago by
Attachment: | trac_8718_codomain.patch added |
---|
consider the codomain in map_coefficients for univariate polynomials
Changed 11 years ago by
Attachment: | trac_8718_multivariate_codomain.patch added |
---|
consider the codomain in map_coefficients for multivariate polynomials
comment:15 Changed 11 years ago by
Description: | modified (diff) |
---|---|
Status: | needs_info → needs_review |
comment:16 Changed 11 years ago by
Work issues: | check if function knows its codomain and use it |
---|
comment:17 Changed 11 years ago by
Dependencies: | → #11981 |
---|
comment:18 Changed 11 years ago by
Cc: | Daniel Krenn added |
---|---|
Reviewers: | → Daniel Krenn |
Status: | needs_review → positive_review |
comment:19 Changed 11 years ago by
Merged in: | → sage-5.0.beta6 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
The attached patch adds naive implementations of apply_map() and of a second similar method, map_coefficients(), modeled after that of multivariate polynomials.
Note that I have as good as no experience with Python or Sage development—so sorry for any newbie errors... and please review carefully! :-)