Opened 10 years ago
Closed 9 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: |
Description (last modified by )
trac_13736-content-take2.patch provides a thin wrapper around the content()
method in pynac.
Apply
Attachments (3)
Change History (19)
comment:1 Changed 10 years ago by
- Status changed from new to needs_review
Changed 10 years ago by
comment:2 Changed 10 years ago by
comment:3 Changed 10 years ago by
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: ↓ 5 Changed 10 years ago by
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 10 years ago by
- 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
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
comment:7 Changed 9 years ago by
- Description modified (diff)
- Status changed from needs_work to needs_review
comment:8 Changed 9 years ago by
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
- 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
instructions for the bot:
apply trac_13736-content-take2.patch trac_13736-unit_primpart.patch
comment:11 Changed 9 years ago by
- 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 9 years ago by
- Status changed from needs_review to positive_review
comment:13 follow-up: ↓ 14 Changed 9 years ago by
So ... it's okay that content
isn't a hidden method?
comment:14 in reply to: ↑ 13 Changed 9 years ago by
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 9 years ago by
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 9 years ago by
- Merged in set to sage-5.11.beta2
- Resolution set to fixed
- Status changed from positive_review to closed
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
andprimpart
methods (which might even be confused with other things in algebra) I'm reluctant for this to go in by itself. Also,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!