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

[WIP] Reconstruction constructor in CoordSys3D #13056

Merged
merged 29 commits into from Aug 12, 2017

Conversation

Projects
None yet
2 participants
@szymag
Contributor

szymag commented Jul 27, 2017

This PR introduces to constructor transformation equation.
These equations will be obtain from rotation_matrix, location (for translations), and for changing type of coordinate system. After obtaining these equations, method for transformation composition will be used.

We need to deal with two situation. When we want to connect our coordinate system (parent coordinate system) and when we want to just set transformation equations.

@@ -98,6 +98,33 @@ def __new__(cls, name, location=None, rotation_matrix=None,
location = Vector.zero
origin = Point(name + '.origin')

This comment has been minimized.

@Upabjojr

Upabjojr Jul 27, 2017

Contributor

What to do here:

  • check that if ŧransformation is None.
  • if it is None, computer transformation from the rotation and traslation. Make identity if they are also None.
  • if it is not None, check that rotation and translation are None
  • transform transformation to a Lambda.
@Upabjojr

Upabjojr Jul 27, 2017

Contributor

What to do here:

  • check that if ŧransformation is None.
  • if it is None, computer transformation from the rotation and traslation. Make identity if they are also None.
  • if it is not None, check that rotation and translation are None
  • transform transformation to a Lambda.

This comment has been minimized.

@szymag

szymag Jul 27, 2017

Contributor

I'm not sure why do that.
You assume that we have to use transformation or (rotation and translation), so transformation should have information about every three transformations.
I think that it could be easier sometimes to split transformation into steps. For example for transformation set cylindrical and for location set some vector.

@szymag

szymag Jul 27, 2017

Contributor

I'm not sure why do that.
You assume that we have to use transformation or (rotation and translation), so transformation should have information about every three transformations.
I think that it could be easier sometimes to split transformation into steps. For example for transformation set cylindrical and for location set some vector.

This comment has been minimized.

@Upabjojr

Upabjojr Jul 27, 2017

Contributor

We do it the general way, no need to complicate the code with special cases. Translations are subsets of transformations. In the special cases of rotation and translations, we could store some markers to remind of the special case (and remember the steps).

@Upabjojr

Upabjojr Jul 27, 2017

Contributor

We do it the general way, no need to complicate the code with special cases. Translations are subsets of transformations. In the special cases of rotation and translations, we could store some markers to remind of the special case (and remember the steps).

@Upabjojr

This comment has been minimized.

Show comment
Hide comment
@Upabjojr

Upabjojr Jul 27, 2017

Contributor

I suggest to return lambda objects from the methods, so you can compose the transformations by simply calculating f(*g(x)).

Contributor

Upabjojr commented Jul 27, 2017

I suggest to return lambda objects from the methods, so you can compose the transformations by simply calculating f(*g(x)).

@szymag

This comment has been minimized.

Show comment
Hide comment
@szymag

szymag Jul 29, 2017

Contributor

I suggest to return lambda objects from the methods, so you can compose the transformations by simply calculating f(*g(x)).

I used your idea in constructor, when rotation_equations and translation_equations are obtained.

Contributor

szymag commented Jul 29, 2017

I suggest to return lambda objects from the methods, so you can compose the transformations by simply calculating f(*g(x)).

I used your idea in constructor, when rotation_equations and translation_equations are obtained.

@@ -109,12 +156,11 @@ def __new__(cls, name, location=None, rotation_matrix=None,
# they may actually be 'coincident' wrt the root system.
if parent is not None:
obj = super(CoordSys3D, cls).__new__(
cls, Symbol(name), location, parent_orient, parent)
cls, Symbol(name), transformation, parent)

This comment has been minimized.

@Upabjojr

Upabjojr Jul 31, 2017

Contributor

transformation need to be defined.

@Upabjojr

Upabjojr Jul 31, 2017

Contributor

transformation need to be defined.

This comment has been minimized.

@Upabjojr

Upabjojr Jul 31, 2017

Contributor

transformation has to be a SymPy type, if we are going to use a lambda, we need to use SymPy's Lambda (with capital L).

@Upabjojr

Upabjojr Jul 31, 2017

Contributor

transformation has to be a SymPy type, if we are going to use a lambda, we need to use SymPy's Lambda (with capital L).

Show outdated Hide outdated sympy/vector/coordsysrect.py
Show outdated Hide outdated sympy/vector/coordsysrect.py
Show outdated Hide outdated sympy/vector/coordsysrect.py
Show outdated Hide outdated sympy/vector/coordsysrect.py
Show outdated Hide outdated sympy/vector/coordsysrect.py
Show outdated Hide outdated sympy/vector/coordsysrect.py
Show outdated Hide outdated sympy/vector/coordsysrect.py
Show outdated Hide outdated sympy/vector/coordsysrect.py
Show outdated Hide outdated sympy/vector/coordsysrect.py

Upabjojr and others added some commits Aug 2, 2017

Update CoordSys3D.
Almost finished constructor.

Lame_coefficinets, transformation, inverse_transfromation are Lambda,
so they can easily depend on desired variables.

Creating base scalars with custom str has the same form as base scalars
created in standard name: class_name + '.' + variable_names.
Show outdated Hide outdated sympy/vector/coordsysrect.py
Show outdated Hide outdated sympy/vector/coordsysrect.py
Show outdated Hide outdated sympy/vector/coordsysrect.py
Show outdated Hide outdated sympy/vector/coordsysrect.py
Show outdated Hide outdated sympy/vector/coordsysrect.py

szymag and others added some commits Aug 3, 2017

Apply comments.
Remove parameter variables from _calculate_lame_coefficinets,
_calculate_inv_trans_equations, _check_orthogonality

All transformation in constructor are Lambda object.
Pass tests from test_coordsysret.py.
Inverse transformation is calculated only when user needs it.
when transformation is choosen from pre-defined ones, inverse is
also set.
Reduce overhead comming from creating transfotmation.
Remove dependency of parameter variables from every method.
Change form of setting transformation. Variable names should be put
together with equations.

Remove simplify in _calculate_lame_coefficients for a while.
Useage of simplify produce errors which isn't resolve yet.
if transformation is None:
transformation = Tuple(rotation_matrix, location)
if isinstance(transformation, Tuple):

This comment has been minimized.

@szymag

szymag Aug 7, 2017

Contributor

Why that?
In this case, we are not able to set transformation as equations with variables. Previously, we could do that. For example:

a = CoordSys3D('a', transformation=Tuple(((x*cos(y), x*sin(y), z)), (x, y, z))) 

But now you assumed that in following manner we're providing rotation and translation transformations.
Look at the code in _connect_to_cartesian method, lines 363:367

@szymag

szymag Aug 7, 2017

Contributor

Why that?
In this case, we are not able to set transformation as equations with variables. Previously, we could do that. For example:

a = CoordSys3D('a', transformation=Tuple(((x*cos(y), x*sin(y), z)), (x, y, z))) 

But now you assumed that in following manner we're providing rotation and translation transformations.
Look at the code in _connect_to_cartesian method, lines 363:367

This comment has been minimized.

@Upabjojr

Upabjojr Aug 7, 2017

Contributor

I thought a bit over the weekend, if we pass a rotation and/or translation, it needs to be stored in transformation and passed to the superclass (SymPy's hashing requirements). If we transform the rotation and/or translation into a Lambda, the code will slow down a lot, because when the class is reconstructed all orthogonality checks and computations are performed (at least, I think that's the problem).

We can still pass the transformation equations by wrapping them in a Lambda.

@Upabjojr

Upabjojr Aug 7, 2017

Contributor

I thought a bit over the weekend, if we pass a rotation and/or translation, it needs to be stored in transformation and passed to the superclass (SymPy's hashing requirements). If we transform the rotation and/or translation into a Lambda, the code will slow down a lot, because when the class is reconstructed all orthogonality checks and computations are performed (at least, I think that's the problem).

We can still pass the transformation equations by wrapping them in a Lambda.

This comment has been minimized.

@Upabjojr

Upabjojr Aug 7, 2017

Contributor

The main problem is that when calling the superclass in the constructor, we need a fixed argument order. The same argument order must return the same object if called on the constructor. This requirement is pretty restrictive on the way we may write the constructor, so we need to choose a precise way to pack the information without losing speed.

@Upabjojr

Upabjojr Aug 7, 2017

Contributor

The main problem is that when calling the superclass in the constructor, we need a fixed argument order. The same argument order must return the same object if called on the constructor. This requirement is pretty restrictive on the way we may write the constructor, so we need to choose a precise way to pack the information without losing speed.

szymag and others added some commits Aug 9, 2017

szymag and others added some commits Aug 10, 2017

Revert order in transformation.
Variables now are first in tuple.

Add support for python's lambda.

Add test for wrong transformation argument in constructor.

Fix indent inconstructor.

@Upabjojr Upabjojr merged commit c08f558 into sympy:master Aug 12, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@szymag szymag deleted the szymag:reconstruction_constructor branch Aug 23, 2017

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