Opened 9 years ago

Closed 8 years ago

#13736 closed enhancement (fixed)

add content method to symbolic expressions

Reported by: burcin Owned by: burcin
Priority: major Milestone: sage-5.11
Component: symbolics Keywords: sd48
Cc: Merged in: sage-5.11.beta2
Authors: Burcin Erocal, Volker Braun Reviewers: Volker Braun, Burcin Erocal
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Attachments (3)

trac_13736-content.patch (1.6 KB) - added by burcin 9 years ago.
trac_13736-content-take2.patch (1.8 KB) - added by burcin 9 years ago.
trac_13736-unit_primpart.patch (5.9 KB) - added by vbraun 8 years ago.
Updated patch

Download all attachments as: .zip

Change History (19)

comment:1 Changed 9 years ago by burcin

  • Status changed from new to needs_review

Changed 9 years ago by burcin

comment:2 Changed 9 years ago by kcrisman

Is "content" a well-known thing (especially for non-polynomials)? I did find the definition in the Ginac documentation, but this should probably be slightly clarified since it will show up in the tab-completion. Especially since this doesn't provide the corresponding unit and primpart methods (which might even be confused with other things in algebra) I'm reluctant for this to go in by itself. Also,

sage: (2*x+4*sin(y)).content(2*x)
2*x + 4*sin(y)

and things like that seem to not really be in the spirit of this. If anything, shouldn't this content be 1? Or should this be disallowed? I don't think it can be considered user error with such minimal doc of an unfamiliar term.

That said, I realize this is needed for #13609 and hence #13729, so I will try to be responsive to any responses!

comment:3 Changed 9 years ago by burcin

Thanks for looking into this.

I can rename the function to _content() and add a warning that the behavior is not well defined. I doubt if we will ever have proper input error checking or consistency with Sage's description of content for various domains. I definitely don't have time to implement this.

comment:4 follow-up: Changed 9 years ago by kcrisman

Okay, this seems like a reasonable compromise - it can be used for the purposes we need it for and not have it be something that unsuspecting new Algebra students will rely on. I assume the _content version will still suffice for the needs in #13609?

comment:5 in reply to: ↑ 4 Changed 9 years ago by burcin

  • Status changed from needs_review to needs_work

Replying to kcrisman:

Okay, this seems like a reasonable compromise - it can be used for the purposes we need it for and not have it be something that unsuspecting new Algebra students will rely on. I assume the _content version will still suffice for the needs in #13609?

Yes, it will do. I will upload a new patch once I settle on the correct solution of #13609. I don't expect to have time for this till next weekend though.

comment:6 Changed 9 years ago by vbraun

This ticket seems to be a the bottom of the dependency chain for a few pynac-related patches... I'd be happy to review it when ready ;-)

Changed 9 years ago by burcin

comment:7 Changed 9 years ago by burcin

  • Description modified (diff)
  • Status changed from needs_work to needs_review

comment:8 Changed 9 years ago by vbraun

It would be less confusing if we also wrap GiNaC unit and primpart and copy in some of the documentation

The methods

     ex ex::unit(const ex & x);
     ex ex::content(const ex & x);
     ex ex::primpart(const ex & x);
     ex ex::primpart(const ex & x, const ex & c);

return the unit part, content part, and primitive polynomial of a
multivariate polynomial with respect to the variable ‘x’ (the
unit part being the sign of the leading coefficient, the content
part being the GCD of the coefficients, and the primitive
polynomial being the input polynomial divided by the unit and
content parts). The second variant of primpart() expects the
previously calculated content part of the polynomial in c, which
enables it to work faster in the case where the content part has
already been computed. The product of unit, content, and
primitive part is the original polynomial.

This is FactorTermsList[poly, x] in Mathematica, so perhaps we can have a non-underscore method that returns all three?

comment:9 Changed 9 years ago by vbraun

  • Description modified (diff)
  • Reviewers set to Volker Braun

I've taken the liberty to write the patch ;-) Feel free to set to positive review if you agree with it.

comment:10 Changed 9 years ago by chapoton

instructions for the bot:

apply trac_13736-content-take2.patch trac_13736-unit_primpart.patch

Changed 8 years ago by vbraun

Updated patch

comment:11 Changed 8 years ago by burcin

  • Authors changed from Burcin Erocal to Burcin Erocal, Volker Braun
  • Keywords sd48 added
  • Reviewers changed from Volker Braun to Volker Braun, Burcin Erocal

Looks good to me. We can switch this to positive review when the patchbot gets around to testing it.

comment:12 Changed 8 years ago by burcin

  • Status changed from needs_review to positive_review

comment:13 follow-up: Changed 8 years ago by kcrisman

So ... it's okay that content isn't a hidden method?

comment:14 in reply to: ↑ 13 Changed 8 years ago by burcin

Replying to kcrisman:

So ... it's okay that content isn't a hidden method?

Having primpart() and unit() available clarifies what these functions do. The answers they return are what one would expect as long as no exotic coefficients are involved. I don't know what would happen if you have a polynomial where the coefficients live in different domains. But this is the case for most symbolics functionality. I say we merge this and see if anyone complains. :)

That said, I don't understand why the patchbot complains about startup time. These patches should not effect that at all.

comment:15 Changed 8 years ago by vbraun

There might be unaccounted-for systematics in the startup timing code like hdd location and cylinder alignment. We are also talking about a tiny increase, too. And the probability is not *that* high. We have seen somewhat unexpected effects before, its not clear where it comes from.

comment:16 Changed 8 years ago by jdemeyer

  • Merged in set to sage-5.11.beta2
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.