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

Make Expr recursively callable. #1299

Merged
merged 2 commits into from May 16, 2012

Conversation

Projects
None yet
5 participants
@Krastanov
Member

Krastanov commented May 15, 2012

Addresses issue 2006. If an expression is called on an argument it traverses
the expression tree and if an atom is a callable it is called on the argument.
If it is not a callable it is left unchanges:

Examples:

>>> (1+x)(y)
1+x

>>> (1+f)(y)
1+f(y)
Make Expr recursively callable.
Addresses issue 2006. If an expression is called on an argument it traverses
the expression tree and if an atom is a callable it is called on the argument.
If it is not a callable it is left unchanges:

Examples:

```
>>> (1+x)(y)
1+x

>>> (1+f)(y)
1+f(y)
```
@Krastanov

This comment has been minimized.

Show comment
Hide comment
@Krastanov

Krastanov May 15, 2012

Member

The automatic testrunner should check this shortly. Hopefully there wont be any problems.

Member

Krastanov commented May 15, 2012

The automatic testrunner should check this shortly. Hopefully there wont be any problems.

@Krastanov

This comment has been minimized.

Show comment
Hide comment
@Krastanov

Krastanov May 15, 2012

Member

SymPy Bot Summary: 🔴 There were test failures.

@Krastanov: Please fix the test failures.

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sY9bQYDA

Interpreter: /usr/bin/python2.6 (2.6.7-final-0)
Architecture: Linux (64-bit)
Cache: yes
Test command: setup.py test
master hash: c0ab76d
branch hash: 81db7a9

Automatic review by SymPy Bot.

Member

Krastanov commented May 15, 2012

SymPy Bot Summary: 🔴 There were test failures.

@Krastanov: Please fix the test failures.

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sY9bQYDA

Interpreter: /usr/bin/python2.6 (2.6.7-final-0)
Architecture: Linux (64-bit)
Cache: yes
Test command: setup.py test
master hash: c0ab76d
branch hash: 81db7a9

Automatic review by SymPy Bot.

@Krastanov

This comment has been minimized.

Show comment
Hide comment
@Krastanov

Krastanov May 15, 2012

Member

It seems that the only thing that fails (however it fails hard) is the physics.mechanics module.

From whom may I ask for help with this module?

Member

Krastanov commented May 15, 2012

It seems that the only thing that fails (however it fails hard) is the physics.mechanics module.

From whom may I ask for help with this module?

@asmeurer

This comment has been minimized.

Show comment
Hide comment
Member

asmeurer commented May 15, 2012

@Krastanov

This comment has been minimized.

Show comment
Hide comment
@Krastanov

Krastanov May 16, 2012

Member

There was some slightly unpythonic code in mechanics. I have just rewritten it in a more idiomatic way and now it works. I have no idea why. @gilbertgede, could you please double check?

@asmeurer, are the rest of the changes (those to the core) appropriate?

Member

Krastanov commented May 16, 2012

There was some slightly unpythonic code in mechanics. I have just rewritten it in a more idiomatic way and now it works. I have no idea why. @gilbertgede, could you please double check?

@asmeurer, are the rest of the changes (those to the core) appropriate?

@asmeurer

View changes

Show outdated Hide outdated sympy/physics/mechanics/essential.py
return esses
except TypeError, e:

This comment has been minimized.

@asmeurer

asmeurer May 16, 2012

Member

This try block is not necessary either. You're just checking if esses is a Function or a list. So do that check directly, rather than masking it in a try, except.

@asmeurer

asmeurer May 16, 2012

Member

This try block is not necessary either. You're just checking if esses is a Function or a list. So do that check directly, rather than masking it in a try, except.

@staticmethod
def _recursive_call(expr_to_call, on_args):
def the_call_method_is_overridden(expr):

This comment has been minimized.

@asmeurer

asmeurer May 16, 2012

Member

Any reason to not just use hasattr?

@asmeurer

asmeurer May 16, 2012

Member

Any reason to not just use hasattr?

This comment has been minimized.

@Krastanov

Krastanov May 16, 2012

Member

Because I must be sure that __call__ is not defined in Expr. Otherwise I will get infinite recursion (I think).

@Krastanov

Krastanov May 16, 2012

Member

Because I must be sure that __call__ is not defined in Expr. Otherwise I will get infinite recursion (I think).

@asmeurer

View changes

Show outdated Hide outdated sympy/physics/mechanics/essential.py
esses = esses.__call__(dynamicsymbols._t)
for i in range(level):
esses = diff(esses, dynamicsymbols._t)
esses = [reduce(diff, [t]*level, e(t)) for e in esses]

This comment has been minimized.

@asmeurer

asmeurer May 16, 2012

Member

You're going to need to import reduce from compatibility for this to work in Python 3.

@asmeurer

asmeurer May 16, 2012

Member

You're going to need to import reduce from compatibility for this to work in Python 3.

@asmeurer

This comment has been minimized.

Show comment
Hide comment
@asmeurer

asmeurer May 16, 2012

Member

Except for the comments I've already given, I don't have any serious issues with this. Note that this is still severly limited because we still cannot combine Functions with other objects, so (1 + f)(x), (1 + sin)(x), (sin + cos)(x), (sin**2)(x), etc. will still not work. But that's because of a separate issue, which is that unapplied Functions are not Expr.

Actually, if you remove your special casing of Symbol, then things like (1 + x)(y) and (x**2)(y) work, where x is a Symbol. So I'm wondering if we go all the way on this if we should not remove Symbol("x")(y) => Function("x")(y) but rather just make that the way that Function works.

Member

asmeurer commented May 16, 2012

Except for the comments I've already given, I don't have any serious issues with this. Note that this is still severly limited because we still cannot combine Functions with other objects, so (1 + f)(x), (1 + sin)(x), (sin + cos)(x), (sin**2)(x), etc. will still not work. But that's because of a separate issue, which is that unapplied Functions are not Expr.

Actually, if you remove your special casing of Symbol, then things like (1 + x)(y) and (x**2)(y) work, where x is a Symbol. So I'm wondering if we go all the way on this if we should not remove Symbol("x")(y) => Function("x")(y) but rather just make that the way that Function works.

@gilbertgede

This comment has been minimized.

Show comment
Hide comment
@gilbertgede

gilbertgede May 16, 2012

Contributor

I started to rewrite dynamicsymbols to make it compatible with your changes, but what you have done looks a lot better than what I have. The changes you made in mechanics look good to me. @asmeurer, is the proper way to check if it is a Function to use hasattr(e, 'is_Function')?

Contributor

gilbertgede commented May 16, 2012

I started to rewrite dynamicsymbols to make it compatible with your changes, but what you have done looks a lot better than what I have. The changes you made in mechanics look good to me. @asmeurer, is the proper way to check if it is a Function to use hasattr(e, 'is_Function')?

@asmeurer

This comment has been minimized.

Show comment
Hide comment
@asmeurer

asmeurer May 16, 2012

Member

Or just e.is_Function if you know it's been sympified (isinstance(e, Function) works fine too).

Member

asmeurer commented May 16, 2012

Or just e.is_Function if you know it's been sympified (isinstance(e, Function) works fine too).

@asmeurer

This comment has been minimized.

Show comment
Hide comment
@asmeurer

asmeurer May 16, 2012

Member

Oh I see, you're talking about the result of symbols(). I would just use isinstance(e, Function) then.

Member

asmeurer commented May 16, 2012

Oh I see, you're talking about the result of symbols(). I would just use isinstance(e, Function) then.

@Krastanov

This comment has been minimized.

Show comment
Hide comment
@Krastanov

Krastanov May 16, 2012

Member

SymPy Bot Summary: ✳️ All tests have passed.

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sY9dsYDA

Interpreter: /usr/bin/python2.6 (2.6.7-final-0)
Architecture: Linux (64-bit)
Cache: yes
Test command: setup.py test
master hash: c0ab76d
branch hash: 9728ef5ad8d1658ee9406d93d1ed449de8e72aa4

Automatic review by SymPy Bot.

Member

Krastanov commented May 16, 2012

SymPy Bot Summary: ✳️ All tests have passed.

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sY9dsYDA

Interpreter: /usr/bin/python2.6 (2.6.7-final-0)
Architecture: Linux (64-bit)
Cache: yes
Test command: setup.py test
master hash: c0ab76d
branch hash: 9728ef5ad8d1658ee9406d93d1ed449de8e72aa4

Automatic review by SymPy Bot.

@asmeurer

This comment has been minimized.

Show comment
Hide comment
@asmeurer

asmeurer May 16, 2012

Member

SymPy Bot Summary: 🔴 There were test failures.

@Krastanov: Please fix the test failures.

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYup0YDA

Interpreter: /usr/local/bin/python2.5 (2.5.0-final-0)
Architecture: Darwin (32-bit)
Cache: yes
Test command: setup.py test
master hash: c0ab76d
branch hash: 9728ef5ad8d1658ee9406d93d1ed449de8e72aa4

Automatic review by SymPy Bot.

Member

asmeurer commented May 16, 2012

SymPy Bot Summary: 🔴 There were test failures.

@Krastanov: Please fix the test failures.

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYup0YDA

Interpreter: /usr/local/bin/python2.5 (2.5.0-final-0)
Architecture: Darwin (32-bit)
Cache: yes
Test command: setup.py test
master hash: c0ab76d
branch hash: 9728ef5ad8d1658ee9406d93d1ed449de8e72aa4

Automatic review by SymPy Bot.

@asmeurer

This comment has been minimized.

Show comment
Hide comment
@asmeurer

asmeurer May 16, 2012

Member

SymPy Bot Summary: ✳️ All tests have passed.

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYu50YDA

Interpreter: /Library/Frameworks/Python.framework/Versions/3.2/bin/python3 (3.2.2-final-0)
Architecture: Darwin (64-bit)
Cache: yes
Test command: ./bin/test; ./bin/doctest
master hash: c0ab76d
branch hash: 9728ef5ad8d1658ee9406d93d1ed449de8e72aa4

Automatic review by SymPy Bot.

Member

asmeurer commented May 16, 2012

SymPy Bot Summary: ✳️ All tests have passed.

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYu50YDA

Interpreter: /Library/Frameworks/Python.framework/Versions/3.2/bin/python3 (3.2.2-final-0)
Architecture: Darwin (64-bit)
Cache: yes
Test command: ./bin/test; ./bin/doctest
master hash: c0ab76d
branch hash: 9728ef5ad8d1658ee9406d93d1ed449de8e72aa4

Automatic review by SymPy Bot.

@smichr

This comment has been minimized.

Show comment
Hide comment
@smichr

smichr May 16, 2012

Member

On Wed, May 16, 2012 at 5:01 AM, Stefan Krastanov
reply@reply.github.com
wrote:

Addresses issue 2006. If an expression is called on an argument it traverses
the expression tree and if an atom is a callable it is called on the argument.
If it is not a callable it is left unchanges:

Examples:

>>> (1+x)(y)
1+x

>>> (1+f)(y)
1+f(y)

To me this is just really weird syntax. In what cases is this used and
why? (Just an FYI request.)

Member

smichr commented May 16, 2012

On Wed, May 16, 2012 at 5:01 AM, Stefan Krastanov
reply@reply.github.com
wrote:

Addresses issue 2006. If an expression is called on an argument it traverses
the expression tree and if an atom is a callable it is called on the argument.
If it is not a callable it is left unchanges:

Examples:

>>> (1+x)(y)
1+x

>>> (1+f)(y)
1+f(y)

To me this is just really weird syntax. In what cases is this used and
why? (Just an FYI request.)

@Krastanov

This comment has been minimized.

Show comment
Hide comment
@Krastanov

Krastanov May 16, 2012

Member

@smichr, If for example you want to build a field out of basis fields:

base_x(point) -> returns point.coord_x

(base_x**2+base_y**2)(point) -> returns x**2+y**2

It may very well not be idiomatic for sympy, but for the moment I like the way it is done.

Member

Krastanov commented May 16, 2012

@smichr, If for example you want to build a field out of basis fields:

base_x(point) -> returns point.coord_x

(base_x**2+base_y**2)(point) -> returns x**2+y**2

It may very well not be idiomatic for sympy, but for the moment I like the way it is done.

@Krastanov

This comment has been minimized.

Show comment
Hide comment
@Krastanov

Krastanov May 16, 2012

Member

@asmeurer, If I remove the special case for Symbol I will get stuff like:

# scalar_field is the application point -> some_function_of_coords_of_point
# for example scalar_field(point) -> r**2 in polar

>>> point = Point(cartesian, x, y)
>>> (z+scalar_field)(point)
z(point) + x**2+y**2

# I definitely do not want that. I would expect
z + x**2 + y**2
Member

Krastanov commented May 16, 2012

@asmeurer, If I remove the special case for Symbol I will get stuff like:

# scalar_field is the application point -> some_function_of_coords_of_point
# for example scalar_field(point) -> r**2 in polar

>>> point = Point(cartesian, x, y)
>>> (z+scalar_field)(point)
z(point) + x**2+y**2

# I definitely do not want that. I would expect
z + x**2 + y**2
@Krastanov

This comment has been minimized.

Show comment
Hide comment
@Krastanov

Krastanov May 16, 2012

Member

@smichr, Another way to think about it is that everything is now a function, however almost always it is a constant function.

Member

Krastanov commented May 16, 2012

@smichr, Another way to think about it is that everything is now a function, however almost always it is a constant function.

Fix some style idioms in physics.mechanics
There were a few calls of the type `f.__call__(arg)` instead of `f(arg)` and a
few complicated loops over `range()` that were easily converted to much shorter
and readable `reduce` calls and list comprehention.

It also fixed somehow a bug with a recent change in the `__call__` method of
Expr.
@Krastanov

This comment has been minimized.

Show comment
Hide comment
@Krastanov

Krastanov May 16, 2012

Member

@asmeurer, I am confused about the tests: the 2.5 one failed because of something unrelated I think, but why 3.2 did not fail?

Anyway, I now addressed your remarks about the mechanics stuff. The automatic test runner should catch the changes soon.

Member

Krastanov commented May 16, 2012

@asmeurer, I am confused about the tests: the 2.5 one failed because of something unrelated I think, but why 3.2 did not fail?

Anyway, I now addressed your remarks about the mechanics stuff. The automatic test runner should catch the changes soon.

@smichr

This comment has been minimized.

Show comment
Hide comment
@smichr

smichr May 16, 2012

Member

I've got no objections.

Member

smichr commented May 16, 2012

I've got no objections.

@Krastanov

This comment has been minimized.

Show comment
Hide comment
@Krastanov

Krastanov May 16, 2012

Member

SymPy Bot Summary: 🔴 There were test failures.

@Krastanov: Please fix the test failures.

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYzu4XDA

Interpreter: /usr/bin/python2.6 (2.6.7-final-0)
Architecture: Linux (64-bit)
Cache: yes
Test command: setup.py test
master hash: a44b0a8
branch hash: 7c60d7e

Automatic review by SymPy Bot.

Member

Krastanov commented May 16, 2012

SymPy Bot Summary: 🔴 There were test failures.

@Krastanov: Please fix the test failures.

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYzu4XDA

Interpreter: /usr/bin/python2.6 (2.6.7-final-0)
Architecture: Linux (64-bit)
Cache: yes
Test command: setup.py test
master hash: a44b0a8
branch hash: 7c60d7e

Automatic review by SymPy Bot.

@Krastanov

This comment has been minimized.

Show comment
Hide comment
@Krastanov

Krastanov May 16, 2012

Member

The test failure seems to be in master.

Member

Krastanov commented May 16, 2012

The test failure seems to be in master.

@smichr

This comment has been minimized.

Show comment
Hide comment
@smichr

smichr May 16, 2012

Member

Hey, nice new pic, @asmeurer ! Congratulations.

Go ahead and commit this if you have no objections.

Member

smichr commented May 16, 2012

Hey, nice new pic, @asmeurer ! Congratulations.

Go ahead and commit this if you have no objections.

@asmeurer

This comment has been minimized.

Show comment
Hide comment
@asmeurer

asmeurer May 16, 2012

Member

Right. I was just pointing out that if unapplied Functions acted like
Symbols, it would work. I agree that if we do this that we
definitely need a distinction between what does and does not get
recursively called.

On May 16, 2012, at 2:33 AM, Stefan Krastanov
reply@reply.github.com
wrote:

@asmeurer, If I remove the special case for Symbol I will get stuff like:

# scalar_field is the application point -> some_function_of_coords_of_point
# for example scalar_field(point) -> r**2 in polar

>>> point = Point(cartesian, x, y)
>>> (z+scalar_field)(point)
z(point) + x**2+y**2

# I definitely do not want that. I would expect
z + x**2 + y**2

Reply to this email directly or view it on GitHub:
#1299 (comment)

Member

asmeurer commented May 16, 2012

Right. I was just pointing out that if unapplied Functions acted like
Symbols, it would work. I agree that if we do this that we
definitely need a distinction between what does and does not get
recursively called.

On May 16, 2012, at 2:33 AM, Stefan Krastanov
reply@reply.github.com
wrote:

@asmeurer, If I remove the special case for Symbol I will get stuff like:

# scalar_field is the application point -> some_function_of_coords_of_point
# for example scalar_field(point) -> r**2 in polar

>>> point = Point(cartesian, x, y)
>>> (z+scalar_field)(point)
z(point) + x**2+y**2

# I definitely do not want that. I would expect
z + x**2 + y**2

Reply to this email directly or view it on GitHub:
#1299 (comment)

@asmeurer

This comment has been minimized.

Show comment
Hide comment
@asmeurer

asmeurer May 16, 2012

Member

I am also confused. I guess 2to3 works differently than I thought with
respect to reduce. Maybe @vperic can comment. It's not clear why we
have reduce in compatibility if it's not needed.

The 2.5 error is Sage and is indeed unrelated.

On May 16, 2012, at 2:55 AM, Stefan Krastanov
reply@reply.github.com
wrote:

@asmeurer, I am confused about the tests: the 2.5 one failed because of something unrelated I think, but why 3.2 did not fail?

Anyway, I now addressed your remarks about the mechanics stuff. The automatic test runner should catch the changes soon.


Reply to this email directly or view it on GitHub:
#1299 (comment)

Member

asmeurer commented May 16, 2012

I am also confused. I guess 2to3 works differently than I thought with
respect to reduce. Maybe @vperic can comment. It's not clear why we
have reduce in compatibility if it's not needed.

The 2.5 error is Sage and is indeed unrelated.

On May 16, 2012, at 2:55 AM, Stefan Krastanov
reply@reply.github.com
wrote:

@asmeurer, I am confused about the tests: the 2.5 one failed because of something unrelated I think, but why 3.2 did not fail?

Anyway, I now addressed your remarks about the mechanics stuff. The automatic test runner should catch the changes soon.


Reply to this email directly or view it on GitHub:
#1299 (comment)

@asmeurer

This comment has been minimized.

Show comment
Hide comment
@asmeurer

asmeurer May 16, 2012

Member

I have no objections if tests are passing. I'd like to see the ability to combine unapplied Functions to give this some real power.

Member

asmeurer commented May 16, 2012

I have no objections if tests are passing. I'd like to see the ability to combine unapplied Functions to give this some real power.

@Krastanov

This comment has been minimized.

Show comment
Hide comment
@Krastanov

Krastanov May 16, 2012

Member

@asmeurer, @smichr The only failing test is a code_quality test in a file that this pull request has not touched. As you have no objections to the code in the pull request I will merge it in about an hour (just so you can raise any final issues if you want).

Member

Krastanov commented May 16, 2012

@asmeurer, @smichr The only failing test is a code_quality test in a file that this pull request has not touched. As you have no objections to the code in the pull request I will merge it in about an hour (just so you can raise any final issues if you want).

@Krastanov

This comment has been minimized.

Show comment
Hide comment
@Krastanov

Krastanov May 16, 2012

Member

@asmeurer I will probably play with making Function an expression in the foreseeable future.

Member

Krastanov commented May 16, 2012

@asmeurer I will probably play with making Function an expression in the foreseeable future.

@Krastanov

This comment has been minimized.

Show comment
Hide comment
@Krastanov

Krastanov May 16, 2012

Member

merging...

Member

Krastanov commented May 16, 2012

merging...

Krastanov added a commit that referenced this pull request May 16, 2012

Merge pull request #1299 from Krastanov/callable_expr
Make Expr recursively callable.

@Krastanov Krastanov merged commit 1dbe4be into sympy:master May 16, 2012

@rlamy

This comment has been minimized.

Show comment
Hide comment
@rlamy

rlamy May 17, 2012

Member

Actually, I was -1 on this, cf. ML.

Member

rlamy commented May 17, 2012

Actually, I was -1 on this, cf. ML.

@rlamy

This comment has been minimized.

Show comment
Hide comment
@rlamy

rlamy May 17, 2012

Member

@asmeurer: IIRC sympy.core.compatibility.reduce is only needed because of a bug in 2to3's handling of doctests. For ordinary code, 2to3 deals with it just fine.

Member

rlamy commented May 17, 2012

@asmeurer: IIRC sympy.core.compatibility.reduce is only needed because of a bug in 2to3's handling of doctests. For ordinary code, 2to3 deals with it just fine.

@Krastanov

This comment has been minimized.

Show comment
Hide comment
@Krastanov

Krastanov May 17, 2012

Member

@rlamy, @asmeurer, I think that this is still the last one merged in master. A revert is fairly easy. If you insist we can make the revert and continue the discussion.

Even without a revert we can continue the discussion. I have answered the message on the mailing list.

Member

Krastanov commented May 17, 2012

@rlamy, @asmeurer, I think that this is still the last one merged in master. A revert is fairly easy. If you insist we can make the revert and continue the discussion.

Even without a revert we can continue the discussion. I have answered the message on the mailing list.

@rlamy

This comment has been minimized.

Show comment
Hide comment
@rlamy

rlamy May 18, 2012

Member

There's no pressing need for a revert. Action on this can wait until the discussion is resolved.

Member

rlamy commented May 18, 2012

There's no pressing need for a revert. Action on this can wait until the discussion is resolved.

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