Opened 11 years ago
Closed 9 years ago
#11839 closed defect (fixed)
sage.symbolic.expression.Expression.collect has no documentation
Reported by: | mjo | Owned by: | mvngu |
---|---|---|---|
Priority: | major | Milestone: | sage-5.6 |
Component: | documentation | Keywords: | |
Cc: | Merged in: | sage-5.6.beta0 | |
Authors: | Michael Orlitzky | Reviewers: | Karl-Dieter Crisman, Travis Scrimshaw |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description
The documentation for Expression.collect() has examples, but doesn't actually tell you what it's supposed to do.
Attachments (1)
Change History (11)
comment:1 Changed 10 years ago by
- Status changed from new to needs_review
comment:2 Changed 10 years ago by
- Reviewers set to Karl-Dieter Crisman
- Status changed from needs_review to needs_work
comment:3 follow-up: ↓ 4 Changed 10 years ago by
I left it kind of simple because I've seen expression equality fail on some "easy" cases. A result of bool(f == g) == False
just means "I couldn't prove that f == g
".
In that regard the test is only correct if it's going to generate very simple (i.e. comparable) expressions. We could go higher than the default of degree 2 probably, but I didn't feel like spending too much time thinking about it, either.
comment:4 in reply to: ↑ 3 Changed 10 years ago by
I left it kind of simple because I've seen expression equality fail on some "easy" cases. A result of
bool(f == g) == False
just means "I couldn't prove thatf == g
".
??? I'd like to see this. I think that the kind of examples that the code I posted would generate shouldn't have that happen - we're not talking complicated polynomials here.
In that regard the test is only correct if it's going to generate very simple (i.e. comparable) expressions. We could go higher than the default of degree 2 probably, but I didn't feel like spending too much time thinking about it, either.
Well, considering that the current tests don't actually generate expressions that have anything to collect, I'd prefer at least something with two variables.
comment:5 Changed 10 years ago by
- Status changed from needs_work to needs_review
I was worried about things like,
sage: bool(1/3*sqrt(2/5) == 2/3*sqrt(1/10)) False
and the fact that the relevant Ginac code calls expand:
// correct for lost fractional arguments and return return x + (*this - x).expand();
but I think I've convinced myself that this won't matter because we'll be expanding zero in that last statement.
Patch has been updated.
comment:6 Changed 10 years ago by
This fixes part of #9046, incidentally.
comment:7 Changed 10 years ago by
- Reviewers changed from Karl-Dieter Crisman to Karl-Dieter Crisman, Travis Scrimshaw
The patchbot and myself gets some small fuzz, however I don't know offhand which patch this needs to be rebased off:
patching file sage/symbolic/expression.pyx Hunk #1 succeeded at 5115 with fuzz 1 (offset 274 lines). now at: sage-trac_11839.patch
Also could you please remove all trailing whitespace and remove the dollar signs $
(the single backticks `
takes it into math mode). Otherwise looks good.
comment:8 Changed 10 years ago by
The fuzz was on the blank line before def collect(...)
. Someone added a tab there.
I've fixed the other issues as well, thanks for taking a look.
comment:9 Changed 10 years ago by
- Status changed from needs_review to positive_review
Looks good to me. Thanks.
comment:10 Changed 9 years ago by
- Merged in set to sage-5.6.beta0
- Resolution set to fixed
- Status changed from positive_review to closed
I think that this is pretty good overall, but the random test is a little weak, because they are very small polys. Check out a few!
Instead,
gives us a more robust one. Can you think of even more crazy ones? We could do arbitrarily wacky ones... but at least this show the collect in action; the current test rarely does, as far as I can tell.