Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add is_series() method and others to symbolic expressions #10859

Closed
vbraun opened this issue Feb 28, 2011 · 22 comments
Closed

Add is_series() method and others to symbolic expressions #10859

vbraun opened this issue Feb 28, 2011 · 22 comments

Comments

@vbraun
Copy link
Member

vbraun commented Feb 28, 2011

In order to get more information out of symbolic expressions, I added is_series() and is_terminating_series() methods. This is really necessary if you want to write functions that accept symbolic series as input.

I also noticed that there are various methods _is_something(), which would likewise be useful but are not "public" because of the leading underscore. The second patch removes the leading underscore from them to make them part of the public API.

Apply

Depends on #11320

Component: symbolics

Keywords: sd31

Author: Volker Braun

Reviewer: Karl-Dieter Crisman, Burcin Erocal

Merged: sage-4.7.1.alpha4

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

@vbraun vbraun added this to the sage-4.7 milestone Feb 28, 2011
@vbraun
Copy link
Member Author

vbraun commented Feb 28, 2011

comment:1

Apply trac_10859_add_is_series_to_symbolic_expression.patch, trac_10859_remove_underscores_from_some_symbolic_expression_methods.patch

@kcrisman
Copy link
Member

comment:2

A few comments/questions:

  • Are there any other symbolic objects that would look like series but not be caught by this? (For instance, ones gotten by doing SR(some Maxima thing that returns a series).) I guess I'm wondering if this is only supposed to find things created by .series() or if it's supposed to catch anything that's a series.
  • Does this include Laurent series?
  • There are a couple weird typos, including via the :meth:`series` method.a
  • The Boolean. Whether ``self`` was constructed by :meth:`series`. should look more like it does for other outputs, or at least have a semicolon or colon or something for both of these functions.
  • I think that the second patch should be part of a separate ticket. Presumably someone thought it was good for the methods to be hidden, so it would be helpful to know why that is.
  • Should we maybe even discuss whether these new methods should be underscored?

I apologize for not having time to give this a proper review, though on a cursory examination it looks fine in terms of the actual code and tests.

@kcrisman
Copy link
Member

Reviewer: Karl-Dieter Crisman

@vbraun
Copy link
Member Author

vbraun commented May 4, 2011

comment:3
  1. GiNac has a special operand for series, and is_series() returns true if and only if the expression is a series in this sense. The output will include an Order() term. Maxima does not have an order term as far as I know:
sage: maxima(sin(x)).taylor(maxima(x), 0, 10)
x-x^3/6+x^5/120-x^7/5040+x^9/362880

so converting maxima series to the symbolic ring necessarily creates ordinary polynomials.

  1. Laurent series are included:
sage: (cos(x)/x).series(x,10)
1*x^(-1) + (-1/2)*x + 1/24*x^3 + (-1/720)*x^5 + 1/40320*x^7 + (-1/3628800)*x^9 + Order(x^10)
sage: _.is_series()
True
  1. I think removing the underscores from other is_something() is in the spirit of this ticket of exposing more introspection capabilities to the user. I don't think there is any argument to be made that the user must not be able to access these functions. Rather, it adds obvious functionality to the public API.

I'll fix some english language typos and upload a revised version.

@burcin
Copy link
Contributor

burcin commented May 4, 2011

comment:4

Replying to @vbraun:

  1. I think removing the underscores from other is_something() is in the spirit of this ticket of exposing more introspection capabilities to the user. I don't think there is any argument to be made that the user must not be able to access these functions. Rather, it adds obvious functionality to the public API.

I agree that these functions should be exposed to the user. I was the one to add them with an underscore in the first place, with the thought that they are only useful if you want to get into the internals of symbolic expressions. But anybody who wants to do any serious programming with symbolics needs them. They should be more visible.

I also appreciate any effort to clean up the public API of symbolic expressions. This has been a mess for a long time. AFAIK, it was written to expose some basic maxima functionality initially, then somehow it was decided to keep this old interface and not use the (much better designed) ginac one.

@vbraun
Copy link
Member Author

vbraun commented May 4, 2011

Updated patch

@vbraun
Copy link
Member Author

vbraun commented May 4, 2011

comment:5

Attachment: trac_10859_add_is_series_to_symbolic_expression.patch.gz

@burcin
Copy link
Contributor

burcin commented May 10, 2011

Changed reviewer from Karl-Dieter Crisman to Karl-Dieter Crisman, Burcin Erocal

@burcin
Copy link
Contributor

burcin commented May 10, 2011

comment:6

Thank you for taking the time to improve symbolics. :)

@vbraun
Copy link
Member Author

vbraun commented May 10, 2011

comment:7

Thanks for having a look. The offending calls to underscored methods were not there in Sage-4.6.2 when I wrote the patch. So I'd argue they demonstrate once more the need for exposing this functionality. I don't think we should formally deprecate the methods since they are private, and nobody outside of the Sage library is supposed to use them.

I'll fix the other issues and will test it against the pynac-0.2.2 update.

@burcin
Copy link
Contributor

burcin commented May 10, 2011

comment:8

I am not a fan of the deprecation policy either, but I have been advertising those functions for a while ![1] and in this case it is not so much trouble to deprecate them.

![1] http://wiki.sagemath.org/symbolics/rewrite?action=AttachFile (which should be attached to a trac ticket)

You just need to do _is_something = deprecated_function_alias(is_something, 'Sage 4.7.1') for each function. deprecated_function_alias is in sage.misc.misc.

@vbraun

This comment has been minimized.

@vbraun
Copy link
Member Author

vbraun commented May 10, 2011

Dependencies: #11320

@vbraun
Copy link
Member Author

vbraun commented May 10, 2011

comment:9

All tests pass!

@burcin
Copy link
Contributor

burcin commented May 10, 2011

comment:10

The patchbot doesn't read the dependencies field. Depends on #11320.

We can switch this to a positive review once the bot agrees. The failing maxima tests reported are not relevant.

@vbraun
Copy link
Member Author

vbraun commented May 10, 2011

comment:11

I think the patch buildbot also won't rebuild it just because you uttered "Depends on ...". Though you can take my word that it builds and doctests fine on Sage-4.7.rc1 ;-)

@burcin
Copy link
Contributor

burcin commented May 24, 2011

comment:12

2 weeks! I totally forgot about this. Sorry for the delay.

@jdemeyer
Copy link
Contributor

jdemeyer commented Jun 7, 2011

comment:14

There are lots of doctest failures because of the DeprecationWarning.

@vbraun
Copy link
Member Author

vbraun commented Jun 14, 2011

comment:15

The maxima interface uses the private _is_... methods at one point, this leads to the large number of doctest failures. The updated patch takes care of that. All doctests pass on Sage-4.7.1-alpha2

@vbraun
Copy link
Member Author

vbraun commented Jun 14, 2011

Changed keywords from none to sd31

@vbraun
Copy link
Member Author

vbraun commented Jun 14, 2011

@jdemeyer
Copy link
Contributor

Merged: sage-4.7.1.alpha4

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

No branches or pull requests

4 participants