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:

Status badges

Description

The documentation for Expression.collect() has examples, but doesn't actually tell you what it's supposed to do.

Attachments (1)

sage-trac_11839.patch (3.1 KB) - added by mjo 10 years ago.
Clean up whitespace, math-mode, and fuzz

Download all attachments as: .zip

Change History (11)

comment:1 Changed 10 years ago by mjo

  • Authors set to Michael Orlitzky
  • Status changed from new to needs_review

comment:2 Changed 10 years ago by kcrisman

  • Reviewers set to Karl-Dieter Crisman
  • Status changed from needs_review to needs_work

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,

sage: base = QQ['y']
sage: polynomials = base['x']
sage: f = SR(polynomials.random_element(8)); g = f.expand().collect(x); print f; print g; bool(f == g)

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.

comment:3 follow-up: Changed 10 years ago by mjo

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 kcrisman

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".

??? 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 mjo

  • 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 kcrisman

This fixes part of #9046, incidentally.

comment:7 Changed 10 years ago by tscrim

  • 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.

Changed 10 years ago by mjo

Clean up whitespace, math-mode, and fuzz

comment:8 Changed 10 years ago by mjo

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 tscrim

  • Status changed from needs_review to positive_review

Looks good to me. Thanks.

comment:10 Changed 9 years ago by jdemeyer

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