Skip to content

Conversation

@skirpichev
Copy link
Contributor

TODO:

  • fix O syntax
  • cleanup new and _eval_subs methods
  • rebase?
  • more tests?
  • symbolic points

Related issues: #7203, #7207, #7231 (partially)

@asmeurer
Copy link
Member

Add some examples to the docstring.

@certik
Copy link
Member

certik commented Sep 3, 2013

Thanks a lot for working on this. Very useful and we should have it in sympy.

@asmeurer
Copy link
Member

asmeurer commented Sep 3, 2013

The early commits in this pull request are from other pull requests, some of which are controversial (or at least I personally haven't been convinced that the mathematics are correct). Particularly for the derivative and the conjugation ones. Are they necessary for later commits?

@asmeurer
Copy link
Member

asmeurer commented Sep 3, 2013

If so, let's finish the discussion on those and merge them first before we merge this one.

@asmeurer
Copy link
Member

asmeurer commented Sep 3, 2013

I guess one of the reasons this has not been implemented yet is the difficulty in making O((x - 1)**3, x, 1) + expand((x - 1)**4) return the right thing. I suppose one can always do it by calling series, though this brings up things like https://code.google.com/p/sympy/issues/detail?id=3638 (probably it's not an issue if its only done for polynomial terms, though). What do you think? Is it an issue that O((x - 1)**3, x, 1) + expand((x - 1)**4) doesn't return O((x - 1)**3, x, 1)?

@asmeurer
Copy link
Member

asmeurer commented Sep 3, 2013

Some more old mailing list discussions that might contain some useful information https://groups.google.com/forum/#!topic/sympy/Fa0Xp8w2LA8 https://groups.google.com/forum/#!topic/sympy/AlFy3-mmMBM (from #61).

@skirpichev
Copy link
Contributor Author

The early commits in this pull request are from other pull requests, some of which are controversial

Yes, we can skip them here, if that is an issue.

What do you think? Is it an issue that O((x - 1)**3, x, 1) + expand((x - 1)**4) doesn't return O((x - 1)**3, x, 1)?

Of course.

@skirpichev
Copy link
Contributor Author

Well, now I'm not so sure about the autoevaluation of O((x - 1)**3, x, 1) + expand((x - 1)**4). It seems, we can fix this, but the code looks too complicated.

Also, we can reproduce this "issue" in the master, e.g. (x+1)**2-(x-1)**2 + O(x).

@asmeurer
Copy link
Member

Yes, let's leave it unevaluated. It's as easy as calling series() on the input again to get it to evaluate (we could even make it easier to call series again on an expression with an O() in it without all the arguments again somehow).

@asmeurer
Copy link
Member

This can't be merged. I guess because the other commits were merged separately.

@skirpichev
Copy link
Contributor Author

This can't be merged.

Fixed. Meantime, I've squashed some commits from others pulls (conjugation, derivatives).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would change "XXX: can't do substitution in the multivariate Order symbol yet" -> "Substitution in multivariate Order symbols not implemented yet" or something like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, new and _eval_subs methods needs a cleanup, it's in todo.

@certik
Copy link
Member

certik commented Sep 20, 2013

Otherwise it looks good. Shouldn't you add more tests for this new functionality? Does this implement expansions about arbitrary points now?

@asmeurer
Copy link
Member

I'm still unsure about the conjugate and derivative changes (especially the conjugate one).

@skirpichev
Copy link
Contributor Author

On Fri, Sep 20, 2013 at 10:04:34AM -0700, Ondřej Čertík wrote:

Shouldn't you add more tests for this new functionality?

May be.

Does this implement expansions about arbitrary points now?

Yes. But only at numeric point, one can't be symbolic, e.g. O(x - a).

@skirpichev
Copy link
Contributor Author

On Fri, Sep 20, 2013 at 10:14:44AM -0700, Aaron Meurer wrote:

I'm still unsure about the conjugate and derivative changes (especially
the conjugate one).

This stuff was removed from the current pr.

@asmeurer
Copy link
Member

This stuff was removed from the current pr.

OK.

Yes. But only at numeric point, one can't be symbolic, e.g. O(x - a).

I didn't realize that. What needs to be implemented to support that? Multivariate order?

@skirpichev
Copy link
Contributor Author

What needs to be implemented to support that? Multivariate order?

I guess so.

@asmeurer
Copy link
Member

But why exactly is that?

@jrioux
Copy link
Member

jrioux commented Oct 28, 2013

I agree with the comments that have already been mentioned, this looks pretty promising.

@skirpichev
Copy link
Contributor Author

But why exactly is that?

Weel. The main reason, I think, is the syntax of Order() for this case.

Currently we can't distinguish between O(expr, x, y), where x and
y are variables for O, and O(expr, x, point), where the last variable (point)
is an expansion point and is a symbol.

This problem with the current O() syntax was noted above. Perhaps,
we should change one to something like the subs() syntax (an iterable
container of (var, point) pairs).

This funny syntax was introduced in the commit 0b13b0f (by @jrioux) and
that's not released yet. So, now is a good time to fix this, but I'm
not sure if I can fix this here in a right time before release.
Julien, any suggestions?

@jrioux
Copy link
Member

jrioux commented Oct 30, 2013

Oh, I've only opted for this syntax to make it forward compatible, but I didn't expect that we would be extending this to arbitrary points. The syntax you suggests sounds reasonable, and we can still make it forward compatible: if only symbols are provided rather than (sym, point) pairs, take Zero as the extension point.

@jrioux
Copy link
Member

jrioux commented Oct 30, 2013

For reference, the change was made in #2268 (the pull request might give more context).

@skirpichev
Copy link
Contributor Author

On Wed, Oct 30, 2013 at 02:31:52AM -0700, Julien Rioux wrote:

Oh, I've only opted for this syntax to make it forward compatible, but I
didn't expect that we would be extending this to arbitrary points. The
syntax you suggests sounds reasonable, and we can still make it forward
compatible: if only symbols are provided rather than (sym, point) pairs,
take Zero as the extension point.

Ok. I'll try to patch this before the next release.

@skirpichev
Copy link
Contributor Author

BTW, I wonder if it's reasonable to keep the current multivariate
Order implementation as is. Currently, there is no definition for the
multivariate O symbol. And, according to
the standard definition - our
O class is broken (there is a related issue). Other multivariate stuff
for series was broken too long time ago. For example, as_leading_term produces:

In [1]: (x+y).as_leading_term(x, y)
Out[1]: y

In [2]: (x+y).as_leading_term(y, x)
Out[2]: x

May be it's a good idea to scrap all this and start with univariate
O symbol? Syntax: O(expr) or (optionaly) O(expr, var) or
(optionaly) O(expr, var, point).

@asmeurer
Copy link
Member

asmeurer commented Nov 5, 2013

Is there code that relies on the multivariate O?

@skirpichev
Copy link
Contributor Author

On Mon, Nov 04, 2013 at 08:23:10PM -0800, Aaron Meurer wrote:

Is there code that relies on the multivariate O?

There isn't. Also, it's not easy to imagine (for me) how
this all could be used outside of the Sympy code.

@asmeurer
Copy link
Member

asmeurer commented Nov 6, 2013

Well assuming it is possible to make some actual (correct) computations on a multivariate order symbol, it could be useful.

@asmeurer
Copy link
Member

@mrocklin this is arguably one reason why we should release before the SciPy tutorial. It changes the series conventions at points != 0 (for example), to a way which is both less confusing, and backwards incompatible. What are your thoughts?

@certik
Copy link
Member

certik commented Jun 15, 2014

We should merge this. It fixes a bug. So of course it is incompatible -
previously sympy was giving incorrect results. Now they are correct.

Sent from my mobile phone.
On Jun 15, 2014 12:37 PM, "Aaron Meurer" notifications@github.com wrote:

@mrocklin https://github.com/mrocklin this is arguably one reason why
we should release before the SciPy tutorial. It changes the series
conventions at points != 0 (for example
#2427 (comment)), to a
way which is both less confusing, and backwards incompatible. What are your
thoughts?


Reply to this email directly or view it on GitHub
#2427 (comment).

@skirpichev
Copy link
Contributor Author

we should fix the series printer to handle series like this

Please explain a little.

@asmeurer
Copy link
Member

It's not really a printer, just a special ordering of Add that orders the terms in reverse when there is an O term present. But for the tan series I pasted, the x/3 term should go before the -pi/6, not at the end before the order.

asmeurer added a commit that referenced this pull request Jun 16, 2014
Order (and series) at point != 0 and oo
@asmeurer asmeurer merged commit 35fe8d8 into sympy:master Jun 16, 2014
@asmeurer
Copy link
Member

Can you update the release notes.

@skirpichev
Copy link
Contributor Author

It's not really a printer, just a special ordering of Add that orders the terms

Oh. I don't think this is possible. At least, unless we change Add to something more sophisticated, e.g. some stream-like structure to represent series expansion...

Can you update the release notes.

Sure.

@certik
Copy link
Member

certik commented Jun 16, 2014

Thanks for fixing this Sergey!

@asmeurer
Copy link
Member

I don't think any special classes are needed. We just need to extend what is there to make it smarter. I opened #7623 for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants