Help fixing sympy.vector.Point

[Aaron Meurer]

I'm trying to fix sympy.vector.Point so that it follows the args
invariant. Right now, there is a check that the parent_point must be a
Point instance, but it can also be None, in which case, it is set to a
Symbol. This causes a Point constructed in such way to not follow
expr.func(*expr.args) == expr.

Example:

In [11]: from sympy.vector import *

In [12]: C = CoordSysCartesian('C')

In [13]: C.origin.func(*C.origin.args)
---------------------------------------------------------------------------
TypeError Traceback (most recent call last)
<ipython-input-13-e75fb47c767f> in <module>()
----> 1 C.origin.func(*C.origin.args)

/Users/aaronmeurer/Documents/Python/sympy/sympy-scratch/sympy/vector/point.py
in __new__(cls, name, position, parent_point)
19 if (not isinstance(parent_point, Point)
20 and parent_point is not None):
---> 21 raise TypeError("parent_point should be a Point instance")
22 #Create an object
23 if parent_point is None:

TypeError: parent_point should be a Point instance

I don't know how to fix this because I don't know what the
parent_point should be set to in this case (I'm not completely clear
on the mathematics there).

THIS IS BLOCKING THE RELEASE, SO IMMEDIATE HELP WOULD BE APPRECIATED.

As a side note, the fact that this sort of thing exists in SymPy is
embarrassingly bad. We should fix test_args to test that objects are
recreatable from their arguments and put a moratorium on new objects
that don't follow that rule.

Aaron Meurer

[Francesco Bonazzi]

On Tuesday, 10 November 2015 00:04:39 UTC+1, Aaron Meurer wrote:

I'm trying to fix sympy.vector.Point so that it follows the args
invariant. Right now, there is a check that the parent_point must be a
Point instance, but it can also be None, in which case, it is set to a
Symbol. This causes a Point constructed in such way to not follow
expr.func(*expr.args) == expr.

There are also some subclassing of Symbol and ignoring the parameter extensions:
https://github.com/sympy/sympy/blob/01b4f8d97be55746ee2abe38163b533ca5318ba7/sympy/vector/scalar.py#L14

index and system are defining parameters, they should be in the .args.

Perhaps one should follow the example in sympy.diffgeom,
https://github.com/sympy/sympy/blob/f3de4f3698c28355d6397e917b3d9d5bbf9c84c0/sympy/diffgeom/diffgeom.py#L467

BaseScalarField in diffgeom is the analogous of BaseField in vector.

Example:

In [11]: from sympy.vector import *

In [12]: C = CoordSysCartesian('C')

If I remember correctly, the letter 'C' (the name of the coordinate system) doesn't get registered in the args, that's another issue.
 

As a side note, the fact that this sort of thing exists in SymPy is
embarrassingly bad. We should fix test_args to test that objects are
recreatable from their arguments and put a moratorium on new objects
that don't follow that rule.

It's a bit complicated to explain to new users how to write class constructors.

Mathematica has a pattern matching syntax that forces you to keep its equivalent of SymPy's args sorted in this particular way.

[Sachin Joglekar]

An attempt to fix the issues here: https://github.com/sympy/sympy/pull/10130 . Also added tests to ensure that we test this part of compatibility with the core, in any further work done to sympy.vector.

[Aaron Meurer]

On Tue, Nov 10, 2015 at 2:21 AM, Francesco Bonazzi
<franz....@gmail.com> wrote:
>
>
> On Tuesday, 10 November 2015 00:04:39 UTC+1, Aaron Meurer wrote:
>>
>> I'm trying to fix sympy.vector.Point so that it follows the args
>> invariant. Right now, there is a check that the parent_point must be a
>> Point instance, but it can also be None, in which case, it is set to a
>> Symbol. This causes a Point constructed in such way to not follow
>> expr.func(*expr.args) == expr.
>
>
> There are also some subclassing of Symbol and ignoring the parameter
> extensions:
> https://github.com/sympy/sympy/blob/01b4f8d97be55746ee2abe38163b533ca5318ba7/sympy/vector/scalar.py#L14
>
> index and system are defining parameters, they should be in the .args.
>
> Perhaps one should follow the example in sympy.diffgeom,
> https://github.com/sympy/sympy/blob/f3de4f3698c28355d6397e917b3d9d5bbf9c84c0/sympy/diffgeom/diffgeom.py#L467
>
> BaseScalarField in diffgeom is the analogous of BaseField in vector.
>
>>
>> Example:
>>
>> In [11]: from sympy.vector import *
>>
>> In [12]: C = CoordSysCartesian('C')
>>
>
> If I remember correctly, the letter 'C' (the name of the coordinate system)
> doesn't get registered in the args, that's another issue.

Yes, I already fixed that at https://github.com/sympy/sympy/pull/10084
(see https://github.com/asmeurer/sympy/commit/4fc85947aeccd78907f320ede0f93f444eb68609).
This is because subclasses of Symbol have some problems with caching
that are leading to test failures in Python 3.5, but regardless, an
object with arguments shouldn't subclass Symbol (it turns out that due
to the way caching works right now, really nothing should subclass
Symbol if it intends to extend it in any way, but even without this
issue, any subclass of Symbol should have empty .args).

>
>>
>>
>> As a side note, the fact that this sort of thing exists in SymPy is
>> embarrassingly bad. We should fix test_args to test that objects are
>> recreatable from their arguments and put a moratorium on new objects
>> that don't follow that rule.
>>
>
> It's a bit complicated to explain to new users how to write class
> constructors.
>
> Mathematica has a pattern matching syntax that forces you to keep its
> equivalent of SymPy's args sorted in this particular way.

We could enforce the "args must be instances of Basic" in the Basic
metaclass. That would be a bit extreme, though, and it would at least
require fixing the existing classes in SymPy first.

The second rule can be enforced for library code in tests. I've
started this at https://github.com/sympy/sympy/pull/10128. I doubt I
will finish it before the release, unless someone else wants to help
out.

Beyond that, we desperately need documentation on how to properly
subclass SymPy objects and how to create your own objects in general.
I opened https://github.com/sympy/sympy/issues/10132.

Aaron Meurer

>
> --
> You received this message because you are subscribed to the Google Groups
> "sympy" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to sympy+un...@googlegroups.com.
> To post to this group, send email to sy...@googlegroups.com.
> Visit this group at http://groups.google.com/group/sympy.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/sympy/90c852e2-911c-4be1-93b0-6798e7736d5e%40googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

[Aaron Meurer]

Thanks for the fix Sachin.

Now I'm hitting a new error. scalar_map calls trigsimp
(https://github.com/sympy/sympy/blob/4bf728685a82c990d48624b482907d3cfa2d1fc2/sympy/vector/coordsysrect.py#L329),
which causes some tests to fail because the matrix changes form (it
starts with sin(q)*sqrt(3) - cos(q) and ends up with -2*cos(q +
pi/3)). The problem is that the expressions otherwise look the same.
It's just the CoordinateSystems buried in the BaseScalar objects that
have different matrices.

You can reproduce the issue by running

./bin/test sympy/vector/tests/test_coordsysrect.py

in my python35 branch (https://github.com/sympy/sympy/pull/10084).

I don't know what the correct fix here. I'm guessing trigsimp should
have been called when the original C was created (in orient_new_axis),
but I'm unsure.

Aaron Meurer

[Aaron Meurer]

This is my proposed patch

diff --git a/sympy/vector/coordsysrect.py b/sympy/vector/coordsysrect.py
index e5125d7..bfb00f9 100644
--- a/sympy/vector/coordsysrect.py
+++ b/sympy/vector/coordsysrect.py
@@ -438,6 +438,7 @@ def orient_new(self, name, orienters, location=None,
final_matrix = orienters.rotation_matrix(self)
else:
final_matrix = orienters.rotation_matrix()
+ final_matrix = trigsimp(final_matrix)
else:
final_matrix = Matrix(eye(3))
for orienter in orienters:

If there are no objections, I am going to apply this at
https://github.com/sympy/sympy/pull/10084, because it fixes the bug in
question.

Aaron Meurer

[Jason Moore]

If it fixes the bug and let's the Python 3.5 compatibility move forward that's fine, but in the long run it will be a nasty slowdown for complex orientation matrices. Can you please add a TODO comment or open an issue that a performance preferable fix is needed?

Jason
moorepants.info
+01 530-601-9791