Add new method that is more intuitive than orient_explicit()

[moorepants]

The mechanics method orient_explicit() takes a direction cosine matrix that is the inverse of what a ReferenceFrame.dcm() outputs and it is confusing. We have to make clear warnings about this everywhere. It was a mistake that we designed it this way (or rather didn't design it).

This shows the issue:

import sympy as sm
import sympy.physics.mechanics as me

cxx, cyy, czz = me.dynamicsymbols('c_{xx}, c_{yy}, c_{zz}')
cxy, cxz, cyx = me.dynamicsymbols('c_{xy}, c_{xz}, c_{yx}')
cyz, czx, czy = me.dynamicsymbols('c_{yz}, c_{zx}, c_{zy}')

B_C_A = sm.Matrix([[cxx, cxy, cxz],
                   [cyx, cyy, cyz],
                   [czx, czy, czz]])

A = me.ReferenceFrame('A')
B = me.ReferenceFrame('B')
B.orient_explicit(A, B_C_A)
B.dcm(A)

returns:

Matrix([
[c_{xx}(t), c_{yx}(t), c_{zx}(t)],
[c_{xy}(t), c_{yy}(t), c_{zy}(t)],
[c_{xz}(t), c_{yz}(t), c_{zz}(t)]])

which is the inverse of the input.

So to get the expected behavior you must do:

B.orient_explicit(A, B_C_A.transpose())
B.dcm(A)

which returns the expected result:

Matrix([
[c_{xx}(t), c_{xy}(t), c_{xz}(t)],
[c_{yx}(t), c_{yy}(t), c_{yz}(t)],
[c_{zx}(t), c_{zy}(t), c_{zz}(t)]])

We should introduce a orient_from_dcm() or similarly named method that works just like orient_explicit() but does the intuitive behavior. We can just leave orient_explicit() in place for perpetuity to avoid breaking people's code.

[NanoNish]

So far as I see, we just need to handle the transposing inside the orient_from_dcm(), right? I can work on this.

[NanoNish]

@moorepants could you confirm my approach to this?

[moorepants]

If we keep orient_explicit, then you can just have the new method call orient_explicit.

[tjstienstra]

I think that having two methods, which do the same but use the transpose, only makes it more confusing. The main problem is that it is not directly clear how the rotation matrix is defined in orient_explicit. Therefore, I would propose clarifying that in the docstring.

[moorepants]

I think that having two methods, which do the same but use the transpose, only makes it more confusing.

We should remove all documentation except the docstring from the orient explicit so it isn't shown in use in examples etc. I would do that with the Learn Mulibody Dynamics text once the new method is in. Also I think pushed for the "explicit" name before and I now don't think that was a good choice. Having dcm in the name fits more with the dcm() method naming.

Therefore, I would propose clarifying that in the docstring.

Also needed. We have examples of this in some of the other orient docstrings that try to clarify the definition.

[tjstienstra]

We should remove all documentation except the docstring from the orient explicit so it isn't shown in use in examples etc.

Would you also propose deprecating it?

Also I think pushed for the "explicit" name before and I now don't think that was a good choice.

This is the comment you are referring to. And I agree that orient_dcm would be clearer.

Concluding I would say that there are two problems, which can indeed be solved at once:

• The naming of orient_explicit is suboptimal and should be improved
• It is not clear how a rotation matrix is defined, i.e. from A to B or from B to A.

[moorepants]

Would you also propose deprecating it?

I'm more of the style of leaving it there so it doesn't break code but hide it. We could remove the docstring from displaying in the online docs, but leave the docstring if someone inspects interactively or in an IDE (because it is still public). It's not like the function is broken, it just stands out as opposite behavior wrt to the other orient and dcm functions.

[smichr]

So could you define orient_dcm and then alias the suboptimally named one as orient_inv_dcm = orient_explicit? And then eventually deprecate orient_explicit?

[tjstienstra]

Concluding from the issue I would propose to do the following things:

• A new method orient_dcm (personal opinion, may also be different):
  • It should have a clear name. Proposed in this issue is orient_from_dcm. Personally I would choose orient_dcm instead, because from seems to me like redundant characters and it matches orient_axis, orient_quaternion, etc a lot better.
  • It should use a dcm describing the rotation from the parent to the child (orient_explicit is the opposite)
  • It should have the full description that is currently in orient_explicit. However the docstring should be updated to:
    • the new rotation direction (this mainly changes the examples a bit)
    • Make the rotation direction more clear. I would propose the following opening sentence: Sets the orientation of this reference frame relative to another (parent) reference frame using a direction cosine matrix that describes the rotation from the parent to the child.
• orient_explicit should be excluded from the online documentation using :exclude-members: orient_explicit

@moorepants do you agree with these changes?

P.S. I would also propose moving the implementation to orient_dcm and let orient_explicit call orient_dcm`. This would make it more consistent with the deprecation policy, though we are not really deprecating it.

[moorepants]

I agree with Timo's proposal here.

[Deep-Rai-08]

hello all I am a beginner here and want to contribute in project can someone please guide me how to approach to a problem.

[dhivyeshrk]

Hi @moorepants! If the issue is still (partially) unsolved, I would be happy to work on it.

[tjstienstra]

The issue is still open. #24768 has never been fully completed.

[harshkasat]

@TJStienstra I have raised a Pull Request. Please let me know if any modifications are needed for the code.