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

Inverse transformation equations #12872

Merged
merged 13 commits into from Jul 7, 2017

Conversation

Projects
None yet
2 participants
@szymag
Contributor

szymag commented Jul 2, 2017

This PR introduce inverse transformation equations into vector module.

@szymag

This comment has been minimized.

Show comment
Hide comment
@szymag

szymag Jul 2, 2017

Contributor

ping @Upabjojr

Contributor

szymag commented Jul 2, 2017

ping @Upabjojr

@Upabjojr

This comment has been minimized.

Show comment
Hide comment
@Upabjojr

Upabjojr Jul 2, 2017

Contributor

Looks good, except for the usage of self.x, self.y, self.z, it should rather be sc.x, sc.y, sc.z. You can either solve it through lambdas, or by creating a standard instance of CoordSys3D to be used as reference.

Contributor

Upabjojr commented Jul 2, 2017

Looks good, except for the usage of self.x, self.y, self.z, it should rather be sc.x, sc.y, sc.z. You can either solve it through lambdas, or by creating a standard instance of CoordSys3D to be used as reference.

self._inv_transformation_eqs = self._calculate_inv_transformation_equations(
self._transformation_equations())
else:
raise ValueError("Wrong set of parameter.")

This comment has been minimized.

@szymag

szymag Jul 3, 2017

Contributor

I'm really dissatisfied from this condition, but at that moment don't have idea how to resolve that problem.

@szymag

szymag Jul 3, 2017

Contributor

I'm really dissatisfied from this condition, but at that moment don't have idea how to resolve that problem.

This comment has been minimized.

@Upabjojr

Upabjojr Jul 3, 2017

Contributor

We can re-address this in the future.

@Upabjojr

Upabjojr Jul 3, 2017

Contributor

We can re-address this in the future.

@szymag

This comment has been minimized.

Show comment
Hide comment
@szymag

szymag Jul 3, 2017

Contributor

OK, maybe the solution will come itself

Contributor

szymag commented Jul 3, 2017

OK, maybe the solution will come itself

@szymag

This comment has been minimized.

Show comment
Hide comment
@szymag

szymag Jul 5, 2017

Contributor

@Upabjojr, I don't know why travis fail. I've run tests locally on my computer many times and everything was OK.
If you don't mind, I'd like to close this PR and open new one.

Contributor

szymag commented Jul 5, 2017

@Upabjojr, I don't know why travis fail. I've run tests locally on my computer many times and everything was OK.
If you don't mind, I'd like to close this PR and open new one.

@Upabjojr

This comment has been minimized.

Show comment
Hide comment
@Upabjojr

Upabjojr Jul 5, 2017

Contributor

@szymag you didn't update all the atan functions.

Contributor

Upabjojr commented Jul 5, 2017

@szymag you didn't update all the atan functions.

@Upabjojr

This comment has been minimized.

Show comment
Hide comment
@Upabjojr

Upabjojr Jul 5, 2017

Contributor

atan2 has the advantage that it determines the correct angle (up to 360 degrees), because y and x are separate arguments, so you have 4 sign combinations (x and y can be positive or negative separately).

The function atan(y / x) is a bit ambiguous, it determines the angle up to 180 degrees. y / x has only two possible signs (positive / negative).

Contributor

Upabjojr commented Jul 5, 2017

atan2 has the advantage that it determines the correct angle (up to 360 degrees), because y and x are separate arguments, so you have 4 sign combinations (x and y can be positive or negative separately).

The function atan(y / x) is a bit ambiguous, it determines the angle up to 180 degrees. y / x has only two possible signs (positive / negative).

@Upabjojr

This comment has been minimized.

Show comment
Hide comment
@Upabjojr

Upabjojr Jul 5, 2017

Contributor
    342     assert a._transformation_equations() == (a.x, a.y, a.z)
    343     assert a.lame_coefficients() == (1, 1, 1)
--> 344     assert a._inverse_transformation_equations() == (a.x, a.y, a.z)
    345     a._connect_to_standard_cartesian(((x, y, z), ((x * cos(y), x * sin(y), z))), inverse=False)
    346     assert a._transformation_equations() == (a.x * cos(a.y), a.x * sin(a.y), a.z)

ipdb> !a._inverse_transformation_equations() 
(a.z, a.y, a.x)
ipdb> !a._inverse_transformation_equations() == (a.x, a.y, a.z)
False
ipdb> !a._inverse_transformation_equations(), (a.x, a.y, a.z)
((a.z, a.y, a.x), (a.x, a.y, a.z))
Contributor

Upabjojr commented Jul 5, 2017

    342     assert a._transformation_equations() == (a.x, a.y, a.z)
    343     assert a.lame_coefficients() == (1, 1, 1)
--> 344     assert a._inverse_transformation_equations() == (a.x, a.y, a.z)
    345     a._connect_to_standard_cartesian(((x, y, z), ((x * cos(y), x * sin(y), z))), inverse=False)
    346     assert a._transformation_equations() == (a.x * cos(a.y), a.x * sin(a.y), a.z)

ipdb> !a._inverse_transformation_equations() 
(a.z, a.y, a.x)
ipdb> !a._inverse_transformation_equations() == (a.x, a.y, a.z)
False
ipdb> !a._inverse_transformation_equations(), (a.x, a.y, a.z)
((a.z, a.y, a.x), (a.x, a.y, a.z))
@Upabjojr

This comment has been minimized.

Show comment
Hide comment
@Upabjojr

Upabjojr Jul 5, 2017

Contributor

@szymag somehow the order of the base functions gets inverted.

Contributor

Upabjojr commented Jul 5, 2017

@szymag somehow the order of the base functions gets inverted.

@szymag

This comment has been minimized.

Show comment
Hide comment
@szymag

szymag Jul 6, 2017

Contributor

There is still problem with the same line, 343 in test_coordsysrect.py.
I will try to open new PR.

Contributor

szymag commented Jul 6, 2017

There is still problem with the same line, 343 in test_coordsysrect.py.
I will try to open new PR.

@szymag

This comment has been minimized.

Show comment
Hide comment
@szymag

szymag Jul 6, 2017

Contributor

I add print to see what is returning in this line. I hope it will works.

Contributor

szymag commented Jul 6, 2017

I add print to see what is returning in this line. I hope it will works.

@szymag

This comment has been minimized.

Show comment
Hide comment
@szymag

szymag Jul 6, 2017

Contributor

Print shows that the output is

(a.z, a.y, a.x)

instead of

(a.x, a.y, a.z)
Contributor

szymag commented Jul 6, 2017

Print shows that the output is

(a.z, a.y, a.x)

instead of

(a.x, a.y, a.z)
@Upabjojr

This comment has been minimized.

Show comment
Hide comment
@Upabjojr

Upabjojr Jul 6, 2017

Contributor

Maybe it's the output of solve? Maybe there is no definite order in its output. There should be an option to get the output as a dictionary.

Contributor

Upabjojr commented Jul 6, 2017

Maybe it's the output of solve? Maybe there is no definite order in its output. There should be an option to get the output as a dictionary.

@Upabjojr

This comment has been minimized.

Show comment
Hide comment
@Upabjojr

Upabjojr Jul 6, 2017

Contributor

I think it's useless to open a new PR, the problem is likely in the code.

Contributor

Upabjojr commented Jul 6, 2017

I think it's useless to open a new PR, the problem is likely in the code.

@szymag

This comment has been minimized.

Show comment
Hide comment
@szymag

szymag Jul 6, 2017

Contributor

Maybe it's the output of solve? Maybe there is no definite order in its output. There should be an option to get the output as a dictionary.

I will try it.
The strangest thing is that on my computer it works correctly and I have the same every file as here.

Contributor

szymag commented Jul 6, 2017

Maybe it's the output of solve? Maybe there is no definite order in its output. There should be an option to get the output as a dictionary.

I will try it.
The strangest thing is that on my computer it works correctly and I have the same every file as here.

@Upabjojr

This comment has been minimized.

Show comment
Hide comment
@Upabjojr

Upabjojr Jul 6, 2017

Contributor

On my computer, I get the error. This can happen sometimes, one common cause is when you have an array that is read out of a dictionary or a set, without sorting. Python does not have a unique way of hashing objects (for example, Python functions are hashed by their memory address, which can vary every time you run the script), so the order may depend on the computer/operating system.

Contributor

Upabjojr commented Jul 6, 2017

On my computer, I get the error. This can happen sometimes, one common cause is when you have an array that is read out of a dictionary or a set, without sorting. Python does not have a unique way of hashing objects (for example, Python functions are hashed by their memory address, which can vary every time you run the script), so the order may depend on the computer/operating system.

@Upabjojr

This comment has been minimized.

Show comment
Hide comment
@Upabjojr

Upabjojr Jul 6, 2017

Contributor

(I mean... elements in dict/set are stored in a Python internal data structure, their hash contributes to the way they are stored, so if you transform a dict into a list or array, the order may depend on the hash).

Contributor

Upabjojr commented Jul 6, 2017

(I mean... elements in dict/set are stored in a Python internal data structure, their hash contributes to the way they are stored, so if you transform a dict into a list or array, the order may depend on the hash).

@szymag

This comment has been minimized.

Show comment
Hide comment
@szymag

szymag Jul 6, 2017

Contributor

On my computer, I get the error.

It's better that this error occurs now.

Contributor

szymag commented Jul 6, 2017

On my computer, I get the error.

It's better that this error occurs now.

@Upabjojr Upabjojr merged commit d2f9e93 into sympy:master Jul 7, 2017

1 check passed

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

@szymag szymag deleted the szymag:inverse_transformation branch Jul 7, 2017

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