sage.symbolic.expression.Expression.collect has no documentation

[orlitzky]

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

Component: documentation

Author: Michael Orlitzky

Reviewer: Karl-Dieter Crisman, Travis Scrimshaw

Merged: sage-5.6.beta0

Issue created by migration from https://trac.sagemath.org/ticket/11839

[orlitzky]

Author: Michael Orlitzky

[kcrisman]

Reviewer: Karl-Dieter Crisman

[kcrisman]

comment:2

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.

[orlitzky]

comment:3

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.

[kcrisman]

comment:4

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.

[orlitzky]

comment:5

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.

[kcrisman]

comment:6

This fixes part of #9046, incidentally.

[tscrim]

comment:7

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.

[tscrim]

Changed reviewer from Karl-Dieter Crisman to Karl-Dieter Crisman, Travis Scrimshaw

[orlitzky]

Attachment: sage-trac_11839.patch.gz

Clean up whitespace, math-mode, and fuzz

[orlitzky]

comment:8

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.

[tscrim]

comment:9

Looks good to me. Thanks.

[jdemeyer]

Merged: sage-5.6.beta0