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 On Wed, Nov 18, 2015 at 2:59 PM, Aaron Meurer <[email protected]> wrote: > 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 > > > On Tue, Nov 10, 2015 at 3:46 PM, Aaron Meurer <[email protected]> wrote: > > 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 > > > > On Tue, Nov 10, 2015 at 6:25 AM, Sachin Joglekar > > <[email protected]> wrote: > >> 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. > >> > >> On Tue, Nov 10, 2015 at 1:51 PM, Francesco Bonazzi < > [email protected]> > >> 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. > >>> > >>>> > >>>> > >>>> 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. > >> > >> > -- 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 [email protected]. To post to this group, send email to [email protected]. 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/CAP7f1AjEp9-GSKFiwvSLWVWzEN52Y5jTMwci1cieaqwFf22-fw%40mail.gmail.com. For more options, visit https://groups.google.com/d/optout.
