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:

Status badges

Description (last modified by Julian Rüth)

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

  1. trac_8718.patch
  2. trac_8718_codomain.patch
  3. trac_8718_multivariate_codomain.patch

to the sage repository.

Attachments (4)

trac_8718_map_over_polynomial.patch (3.4 KB) - added by Marc Mezzarobba 13 years ago.
trac_8718.patch (1.6 KB) - added by Julian Rüth 11 years ago.
adds map_coefficients()
trac_8718_codomain.patch (2.7 KB) - added by Julian Rüth 11 years ago.
consider the codomain in map_coefficients for univariate polynomials
trac_8718_multivariate_codomain.patch (3.6 KB) - added by Julian Rüth 11 years ago.
consider the codomain in map_coefficients for multivariate polynomials

Download all attachments as: .zip

Change History (23)

Changed 13 years ago by Marc Mezzarobba

comment:1 Changed 13 years ago by Marc Mezzarobba

Status: newneeds_review

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! :-)

comment:2 Changed 13 years ago by Jason Grout

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 Marc Mezzarobba

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 Jason Grout

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 David Loeffler

Status: needs_reviewneeds_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 Daniel Krenn

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 Julian Rüth

Authors: Marc Mezzarobba, Julian Rueth
Milestone: sage-wishlistsage-5.0
Status: needs_workneeds_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.

Changed 11 years ago by Julian Rüth

Attachment: trac_8718.patch added

adds map_coefficients()

comment:8 Changed 11 years ago by Julian Rüth

Description: modified (diff)

apply trac_8718.patch

comment:9 Changed 11 years ago by Marc Mezzarobba

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 in reply to:  9 Changed 11 years ago by Julian Rüth

[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 Daniel Krenn

Status: needs_reviewneeds_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 Changed 11 years ago by Julian Rüth

Status: needs_workneeds_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 in reply to:  12 Changed 11 years ago by Daniel Krenn

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 Julian Rüth

Ok. I already have that implemented. Just need to add some more doctests and upload it.

Changed 11 years ago by Julian Rüth

Attachment: trac_8718_codomain.patch added

consider the codomain in map_coefficients for univariate polynomials

Changed 11 years ago by Julian Rüth

consider the codomain in map_coefficients for multivariate polynomials

comment:15 Changed 11 years ago by Julian Rüth

Description: modified (diff)
Status: needs_infoneeds_review

comment:16 Changed 11 years ago by Julian Rüth

Work issues: check if function knows its codomain and use it

comment:17 Changed 11 years ago by Daniel Krenn

Dependencies: #11981

comment:18 Changed 11 years ago by Daniel Krenn

Cc: Daniel Krenn added
Reviewers: Daniel Krenn
Status: needs_reviewpositive_review

comment:19 Changed 11 years ago by Jeroen Demeyer

Merged in: sage-5.0.beta6
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.