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 transform_to method to CoordSys3D #13172

Merged
merged 2 commits into from Aug 24, 2017

Conversation

Projects
None yet
2 participants
@szymag
Contributor

szymag commented Aug 22, 2017

This PR adds a method transform_to which is similar to method that works for translation and rotation.
I also add to documentation short description about that.

This PR also fixes transformation_from_parent method. Coordsys3D doesn't have attribute parent but has_parent`.

@szymag

This comment has been minimized.

Show comment
Hide comment
@szymag

szymag Aug 22, 2017

Contributor

ping @Upabjojr

Contributor

szymag commented Aug 22, 2017

ping @Upabjojr

@Upabjojr

This comment has been minimized.

Show comment
Hide comment
@Upabjojr

Upabjojr Aug 22, 2017

Contributor

This looks OK.

Contributor

Upabjojr commented Aug 22, 2017

This looks OK.

Add transform_to method.
Replace self.parent by self._parent.
Update documentation.
@Upabjojr

This comment has been minimized.

Show comment
Hide comment
@Upabjojr

Upabjojr Aug 23, 2017

Contributor

I was thinking about the transform_to function name. Are we sure it's a good name?

Contributor

Upabjojr commented Aug 23, 2017

I was thinking about the transform_to function name. Are we sure it's a good name?

@Upabjojr

This comment has been minimized.

Show comment
Hide comment
@Upabjojr

Upabjojr Aug 23, 2017

Contributor

The name transform_to makes me think about a way to transform something into a new coordinate system, not about the creation of a new coordinate system. Maybe we should think about a better name.

Contributor

Upabjojr commented Aug 23, 2017

The name transform_to makes me think about a way to transform something into a new coordinate system, not about the creation of a new coordinate system. Maybe we should think about a better name.

@szymag

This comment has been minimized.

Show comment
Hide comment
@szymag

szymag Aug 23, 2017

Contributor

I'm open for discussion.
What about transform_new, like locate_new?

Contributor

szymag commented Aug 23, 2017

I'm open for discussion.
What about transform_new, like locate_new?

@Upabjojr

This comment has been minimized.

Show comment
Hide comment
@Upabjojr

Upabjojr Aug 23, 2017

Contributor

I think that the word transform sounds a bit like something that changes. We need to make sure that it's a new coordinate system that is being created. What about create_new?

Contributor

Upabjojr commented Aug 23, 2017

I think that the word transform sounds a bit like something that changes. We need to make sure that it's a new coordinate system that is being created. What about create_new?

@szymag

This comment has been minimized.

Show comment
Hide comment
@szymag

szymag Aug 23, 2017

Contributor

I like create_new but now it's too general name IMO. Here we can set only transformation. I'd say that in that case we should be able to set every possible parameters

Contributor

szymag commented Aug 23, 2017

I like create_new but now it's too general name IMO. Here we can set only transformation. I'd say that in that case we should be able to set every possible parameters

@Upabjojr

This comment has been minimized.

Show comment
Hide comment
@Upabjojr

Upabjojr Aug 23, 2017

Contributor

Well, isn't the transformation the same as every possible parameter?

Contributor

Upabjojr commented Aug 23, 2017

Well, isn't the transformation the same as every possible parameter?

@szymag

This comment has been minimized.

Show comment
Hide comment
@szymag

szymag Aug 23, 2017

Contributor

Not at that time. We have also rotation_matrix and location.
If we remove them in the future and leave only transformation, create_new will OK

Contributor

szymag commented Aug 23, 2017

Not at that time. We have also rotation_matrix and location.
If we remove them in the future and leave only transformation, create_new will OK

@Upabjojr

This comment has been minimized.

Show comment
Hide comment
@Upabjojr

Upabjojr Aug 23, 2017

Contributor

Not at that time. We have also rotation_matrix and location.
If we remove them in the future and leave only transformation, create_new will OK

Well, everything is eventually stored into transformation eventually. Did you have a look at my code refactory?

I believe create_new is a good name candidate. Do you have any other suggestion?

Contributor

Upabjojr commented Aug 23, 2017

Not at that time. We have also rotation_matrix and location.
If we remove them in the future and leave only transformation, create_new will OK

Well, everything is eventually stored into transformation eventually. Did you have a look at my code refactory?

I believe create_new is a good name candidate. Do you have any other suggestion?

@szymag

This comment has been minimized.

Show comment
Hide comment
@szymag

szymag Aug 23, 2017

Contributor

. Did you have a look at my code refactory?

Yes

I'm talking from user perspective. create_new means that I should be able to create any desired coordinate system. Now I can't set rotation_matrix if I want. To do that I need to use another methods.

Contributor

szymag commented Aug 23, 2017

. Did you have a look at my code refactory?

Yes

I'm talking from user perspective. create_new means that I should be able to create any desired coordinate system. Now I can't set rotation_matrix if I want. To do that I need to use another methods.

@szymag

This comment has been minimized.

Show comment
Hide comment
@szymag

szymag Aug 23, 2017

Contributor

I believe that we should reflect in the name that we're working with transformation equations.

Contributor

szymag commented Aug 23, 2017

I believe that we should reflect in the name that we're working with transformation equations.

@Upabjojr

This comment has been minimized.

Show comment
Hide comment
@Upabjojr

Upabjojr Aug 23, 2017

Contributor

Now I can't set rotation_matrix if I want. To do that I need to use another methods.

Yes, you can.

Contributor

Upabjojr commented Aug 23, 2017

Now I can't set rotation_matrix if I want. To do that I need to use another methods.

Yes, you can.

@Upabjojr

This comment has been minimized.

Show comment
Hide comment
@Upabjojr

Upabjojr Aug 23, 2017

Contributor

You can set transformation=(rotation_matrix, [0, 0, 0])

Contributor

Upabjojr commented Aug 23, 2017

You can set transformation=(rotation_matrix, [0, 0, 0])

@szymag

This comment has been minimized.

Show comment
Hide comment
@szymag

szymag Aug 23, 2017

Contributor

But It's not the clearest way to do that, especially for user who worked with vector package for longer time. As I said, this name will be OK when transformation will only one way for creating transformation.

Contributor

szymag commented Aug 23, 2017

But It's not the clearest way to do that, especially for user who worked with vector package for longer time. As I said, this name will be OK when transformation will only one way for creating transformation.

@Upabjojr

This comment has been minimized.

Show comment
Hide comment
@Upabjojr

Upabjojr Aug 23, 2017

Contributor

what name do you suggest?

Contributor

Upabjojr commented Aug 23, 2017

what name do you suggest?

@szymag

This comment has been minimized.

Show comment
Hide comment
@szymag

szymag Aug 23, 2017

Contributor

I suggested but you didn't like it:)

Contributor

szymag commented Aug 23, 2017

I suggested but you didn't like it:)

@Upabjojr

This comment has been minimized.

Show comment
Hide comment
@Upabjojr

Upabjojr Aug 23, 2017

Contributor

I am not sure about using the word transform.

Contributor

Upabjojr commented Aug 23, 2017

I am not sure about using the word transform.

@szymag

This comment has been minimized.

Show comment
Hide comment
@szymag

szymag Aug 23, 2017

Contributor

I understand your concern.
Would you like to keep in constructor only transformation and remove in the future rotation_matrix and location?
If so, I think we can use create_new

Contributor

szymag commented Aug 23, 2017

I understand your concern.
Would you like to keep in constructor only transformation and remove in the future rotation_matrix and location?
If so, I think we can use create_new

@Upabjojr

This comment has been minimized.

Show comment
Hide comment
@Upabjojr

Upabjojr Aug 23, 2017

Contributor

we could add a parser for transformation=matrix and transformation=3 element list for the translation.

Contributor

Upabjojr commented Aug 23, 2017

we could add a parser for transformation=matrix and transformation=3 element list for the translation.

@Upabjojr Upabjojr merged commit 837a67d into sympy:master Aug 24, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment