Repository: sympy/sympy
Pull request: changing orient_explicit method to orient_dcm method, also adding new method orient_explicit
<!-- Your title above should be a short description of what
was changed. Do not include the issue number in the title. -->

#### References to other Issues or PRs
<!-- If this pull request fixes an issue, write "Fixes #NNNN" in that exact
format, e.g. "Fixes #1234" (see
https://tinyurl.com/auto-closing for more information). Also, please
write a comment on that issue linking back to this pull request once it is
open. -->
Fixes #24764   

#### Brief description of what is fixed or changed
Complete as you mention above :
- [x] change method name `orient_explicit` to `orient_dcm`
- [x] create another method `orient_explicit` for backward compatibility
- [x] mention about rotation matrix direction 
- [x] Also `exclude-members: orient_explicit `

#### Other comments


#### Release Notes

<!-- Write the release notes for this release below between the BEGIN and END
statements. The basic format is a bulleted list with the name of the subpackage
and the release note for this PR. For example:

* solvers
  * Added a new solver for logarithmic equations.

* functions
  * Fixed a bug with log of integers. Formerly, `log(-x)` incorrectly gave `-log(x)`.

* physics.units
  * Corrected a semantical error in the conversion between volt and statvolt which
    reported the volt as being larger than the statvolt.

or if no release note(s) should be included use:

 

See https://github.com/sympy/sympy/wiki/Writing-Release-Notes for more
information on how to write release notes. The bot will check your release
notes automatically to see if they are formatted correctly. -->

<!-- BEGIN RELEASE NOTES -->
* physics.vector
  * Implemented `ReferenceFrame.orient_dcm` to match the direction `ReferenceFrame.dcm`.

<!-- END RELEASE NOTES -->


Problem statement:
Add new method that is more intuitive than orient_explicit()
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:

```python
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:

```python
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.

Hints from issue discussion:
So far as I see, we just need to handle the transposing inside the `orient_from_dcm()`, right? I can work on this.
@moorepants could you confirm my approach to this?
If we keep orient_explicit, then you can just have the new method call orient_explicit.
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.
> 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.
> 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](https://github.com/sympy/sympy/pull/20318#discussion_r552580815) 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.
> 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.
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`?
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.
I agree with Timo's proposal here.

Commit message: Adding .mailmap

Patch to review:
diff --git a/.mailmap b/.mailmap
index 34ec49129623..95b414ba51c1 100644
--- a/.mailmap
+++ b/.mailmap
@@ -654,6 +654,7 @@ Harold Erbin <harold.erbin@gmail.com>
 Harrison Oates <48871176+HarrisonOates@users.noreply.github.com>
 Harry Mountain <harrymountain1@icloud.com>
 Harry Zheng <harry@harryzheng.com>
+Harsh Kasat <hkasat@waymore.io>
 Harsh Agarwal <hagarwal9200@gmail.com>
 Harsh Gupta <mail@hargup.in> <gupta.harsh96@gmail.com>
 Harsh Jain <harshjniitr@gmail.com>
diff --git a/doc/src/modules/physics/vector/api/classes.rst b/doc/src/modules/physics/vector/api/classes.rst
index 7e2e10c07948..74be80ed6df1 100644
--- a/doc/src/modules/physics/vector/api/classes.rst
+++ b/doc/src/modules/physics/vector/api/classes.rst
@@ -7,6 +7,8 @@ Essential Classes
 
 .. autoclass:: sympy.physics.vector.frame.ReferenceFrame
    :members:
+   :exclude-members: orient_explicit
+   :exclude-members: orient_dcm
 
 .. autoclass:: sympy.physics.vector.vector.Vector
    :members:
diff --git a/sympy/physics/vector/frame.py b/sympy/physics/vector/frame.py
index e93435188e22..3372c2afe7df 100644
--- a/sympy/physics/vector/frame.py
+++ b/sympy/physics/vector/frame.py
@@ -720,57 +720,38 @@ def orient_explicit(self, parent, dcm):
         dcm : Matrix, shape(3, 3)
             Direction cosine matrix that specifies the relative rotation
             between the two reference frames.
-
+        rotation matrix : Matrix Rotation Direction
+            The simple matrix rotation relative ``parent`` to ``dcm``.
         Warns
         ======
 
         UserWarning
             If the orientation creates a kinematic loop.
 
-        Examples
-        ========
-
-        Setup variables for the examples:
-
-        >>> from sympy import symbols, Matrix, sin, cos
-        >>> from sympy.physics.vector import ReferenceFrame
-        >>> q1 = symbols('q1')
-        >>> A = ReferenceFrame('A')
-        >>> B = ReferenceFrame('B')
-        >>> N = ReferenceFrame('N')
-
-        A simple rotation of ``A`` relative to ``N`` about ``N.x`` is defined
-        by the following direction cosine matrix:
+        """
+        self.orient_dcm(parent = parent, dcm = dcm)
 
-        >>> dcm = Matrix([[1, 0, 0],
-        ...               [0, cos(q1), -sin(q1)],
-        ...               [0, sin(q1), cos(q1)]])
-        >>> A.orient_explicit(N, dcm)
-        >>> A.dcm(N)
-        Matrix([
-        [1,       0,      0],
-        [0,  cos(q1), sin(q1)],
-        [0, -sin(q1), cos(q1)]])
+    def orient_dcm(self, parent, dcm):
+        """Sets the orientation of this reference frame relative to a parent
+        reference frame by explicitly setting the direction cosine matrix.
 
-        This is equivalent to using ``orient_axis()``:
+        Parameters
+        ==========
 
-        >>> B.orient_axis(N, N.x, q1)
-        >>> B.dcm(N)
-        Matrix([
-        [1,       0,      0],
-        [0,  cos(q1), sin(q1)],
-        [0, -sin(q1), cos(q1)]])
+        parent : ReferenceFrame
+            Reference frame that this reference frame will be rotated relative
+            to.
+        dcm : Matrix, shape(3, 3)
+            Direction cosine matrix that specifies the relative rotation
+            between the two reference frames.
+        rotation matrix : Matrix Rotation Direction
+            The simple matrix rotation relative ``parent`` to ``dcm``.
+        Warns
+        ======
 
-        **Note carefully that** ``N.dcm(B)`` **(the transpose) would be passed
-        into** ``orient_explicit()`` **for** ``A.dcm(N)`` **to match**
-        ``B.dcm(N)``:
+        UserWarning
+            If the orientation creates a kinematic loop.
 
-        >>> A.orient_explicit(N, N.dcm(B))
-        >>> A.dcm(N)
-        Matrix([
-        [1,       0,      0],
-        [0,  cos(q1), sin(q1)],
-        [0, -sin(q1), cos(q1)]])
 
         """
 
@@ -780,9 +761,7 @@ def orient_explicit(self, parent, dcm):
         if not isinstance(dcm, MatrixBase):
             raise TypeError("Amounts must be a SymPy Matrix type object.")
 
-        parent_orient_dcm = dcm
-
-        self._dcm(parent, parent_orient_dcm)
+        self._dcm(parent, dcm.T)
 
         wvec = self._w_diff_dcm(parent)
         self._ang_vel_dict.update({parent: wvec})
@@ -1202,7 +1181,7 @@ def orient(self, parent, rot_type, amounts, rot_order=''):
             self.orient_axis(parent, amounts[1], amounts[0])
 
         elif rot_type == 'DCM':
-            self.orient_explicit(parent, amounts)
+            self.orient_dcm(parent, amounts)
 
         elif rot_type == 'BODY':
             self.orient_body_fixed(parent, amounts, rot_order)
@@ -1313,7 +1292,7 @@ def orientnew(self, newname, rot_type, amounts, rot_order='',
             newframe.orient_axis(self, amounts[1], amounts[0])
 
         elif rot_type == 'DCM':
-            newframe.orient_explicit(self, amounts)
+            newframe.orient_dcm(self, amounts)
 
         elif rot_type == 'BODY':
             newframe.orient_body_fixed(self, amounts, rot_order)
diff --git a/sympy/physics/vector/tests/test_frame.py b/sympy/physics/vector/tests/test_frame.py
index 8e6e2cb1cab7..2550ce990a2f 100644
--- a/sympy/physics/vector/tests/test_frame.py
+++ b/sympy/physics/vector/tests/test_frame.py
@@ -451,10 +451,10 @@ def test_dcm_diff_16824():
     assert simplify(AwB.dot(A.y) - alpha2) == 0
     assert simplify(AwB.dot(B.y) - beta2) == 0
 
-def test_orient_explicit():
+def test_orient_dcm():
     A = ReferenceFrame('A')
     B = ReferenceFrame('B')
-    A.orient_explicit(B, eye(3))
+    A.orient_dcm(B, eye(3))
     assert A.dcm(B) == Matrix([[1, 0, 0], [0, 1, 0], [0, 0, 1]])
 
 def test_orient_axis():


Code review comments:
diff_hunk: @@ -720,57 +720,38 @@ def orient_explicit(self, parent, dcm):
         dcm : Matrix, shape(3, 3)
             Direction cosine matrix that specifies the relative rotation
             between the two reference frames.
-
+        rotation matrix : Matrix Rotation Direction
+            The simple matrix rotation relative ``parent`` to ``dcm``.
         Warns
         ======
 
         UserWarning
             If the orientation creates a kinematic loop.
 
-        Examples
-        ========
-
-        Setup variables for the examples:
-
-        >>> from sympy import symbols, Matrix, sin, cos
-        >>> from sympy.physics.vector import ReferenceFrame
-        >>> q1 = symbols('q1')
-        >>> A = ReferenceFrame('A')
-        >>> B = ReferenceFrame('B')
-        >>> N = ReferenceFrame('N')
-
-        A simple rotation of ``A`` relative to ``N`` about ``N.x`` is defined
-        by the following direction cosine matrix:
+        """
+        self.orient_dcm(parent = parent, dcm = dcm)
path: sympy/physics/vector/frame.py
text: @user1:
Should be `dcm.T`

diff_hunk: @@ -1313,7 +1292,7 @@ def orientnew(self, newname, rot_type, amounts, rot_order='',
             newframe.orient_axis(self, amounts[1], amounts[0])
 
         elif rot_type == 'DCM':
-            newframe.orient_explicit(self, amounts)
+            newframe.orient_dcm(self, amounts)
path: sympy/physics/vector/frame.py
text: @user1:
Same here, this is backward incompatible.

diff_hunk: @@ -451,10 +451,10 @@ def test_dcm_diff_16824():
     assert simplify(AwB.dot(A.y) - alpha2) == 0
     assert simplify(AwB.dot(B.y) - beta2) == 0
 
-def test_orient_explicit():
path: sympy/physics/vector/tests/test_frame.py
text: @user1:
Don't change the `orient_explicit` test because we still want to make sure it works. Instead, add a new `test_orient_dcm`, where you showcase that [this](https://github.com/sympy/sympy/issues/24764#issue-1596398197) works.

diff_hunk: @@ -1202,7 +1181,7 @@ def orient(self, parent, rot_type, amounts, rot_order=''):
             self.orient_axis(parent, amounts[1], amounts[0])
 
         elif rot_type == 'DCM':
-            self.orient_explicit(parent, amounts)
+            self.orient_dcm(parent, amounts)
path: sympy/physics/vector/frame.py
text: @user1:
I don't think we can make this change, as this will break existing code.

diff_hunk: @@ -720,57 +720,38 @@ def orient_explicit(self, parent, dcm):
         dcm : Matrix, shape(3, 3)
             Direction cosine matrix that specifies the relative rotation
             between the two reference frames.
-
+        rotation matrix : Matrix Rotation Direction
+            The simple matrix rotation relative ``parent`` to ``dcm``.
path: sympy/physics/vector/frame.py
text: @user1:
This sentence is unclear.

@user2:
In this package we use the word "direction cosine matrix" instead of "rotation matrix". They are the same thing, so why would we need two?

@author:
```
def orient_dcm(self, parent, dcm):
        """ Sets the orientation of this reference frame relative to a parent
        reference frame by explicitly setting the direction cosine matrix.

        Parameters
        ==========

        parent: ReferenceFrame
            Reference frame that this reference frame will be rotated relative
            to.
        dcm : Matrix, shape(3, 3)
            Direction cosine matrix that specifies the relative rotation
            between the two reference frames.
        rotation matrix: Direction Cosine Matrix
            Rotate relative to ``parent``.
```
* When I read [reference](https://moorepants.github.io/learn-multibody-dynamics/orientation.html#orientation-simple-successive), I understand that ``B.orient_dcm(A, B_C_A)`` or `B.orient_dcm(A, B_C_A)` means that B (reference frame) is relatively rotated to A (reference frame). We can verify using `B.dcm(A)` .
*  Please let me know if any modifications or corrections are needed for the code.
* Sorry for the spam message. I'm new to open source and apologize for any inconvenience. I'm providing:

@user2:
The docstring implies you have added a new argument, but actually it is unclear.

@author:
```
    def orient_dcm(self, parent, dcm):
        """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.

        Parameters
        ==========

        parent : ReferenceFrame
            Reference frame that this reference frame will be rotated relative
            to.
        dcm : Matrix, shape(3, 3)
            Direction cosine matrix that specifies the relative rotation
            between the two reference frames.

        Warns
        ======

```
I think the current docstring implies that a change is needed. Please update me if any change is need.

@user2:
I am referring to this addition:

```
        rotation matrix: Direction Cosine Matrix
            Rotate relative to ``parent``.
```

Is this a new argument? It is formatted in that way.

@author:
There is no new argument. I made a mistake by writing 'rotation matrix' as an argument. 
Here, I have explained about rotation instead.
>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.

Please update me if any change is need.

@user3:
Okay, could you then remove the `rotation matrix` argument and use something as you propose [here](https://github.com/sympy/sympy/pull/25916/files#r1402041601). Make sure that it is clear that the DCM describes the orientation of the parent with respect to the child in `orient_explicit`.

diff_hunk: @@ -720,57 +720,38 @@ def orient_explicit(self, parent, dcm):
         dcm : Matrix, shape(3, 3)
             Direction cosine matrix that specifies the relative rotation
             between the two reference frames.
-
+        rotation matrix : Matrix Rotation Direction
+            The simple matrix rotation relative ``parent`` to ``dcm``.
         Warns
         ======
 
         UserWarning
             If the orientation creates a kinematic loop.
 
-        Examples
-        ========
-
-        Setup variables for the examples:
-
-        >>> from sympy import symbols, Matrix, sin, cos
-        >>> from sympy.physics.vector import ReferenceFrame
-        >>> q1 = symbols('q1')
-        >>> A = ReferenceFrame('A')
-        >>> B = ReferenceFrame('B')
-        >>> N = ReferenceFrame('N')
-
-        A simple rotation of ``A`` relative to ``N`` about ``N.x`` is defined
-        by the following direction cosine matrix:
+        """
+        self.orient_dcm(parent = parent, dcm = dcm)
 
-        >>> dcm = Matrix([[1, 0, 0],
-        ...               [0, cos(q1), -sin(q1)],
-        ...               [0, sin(q1), cos(q1)]])
-        >>> A.orient_explicit(N, dcm)
-        >>> A.dcm(N)
-        Matrix([
-        [1,       0,      0],
-        [0,  cos(q1), sin(q1)],
-        [0, -sin(q1), cos(q1)]])
+    def orient_dcm(self, parent, dcm):
+        """Sets the orientation of this reference frame relative to a parent
+        reference frame by explicitly setting the direction cosine matrix.
 
-        This is equivalent to using ``orient_axis()``:
+        Parameters
+        ==========
 
-        >>> B.orient_axis(N, N.x, q1)
-        >>> B.dcm(N)
-        Matrix([
-        [1,       0,      0],
-        [0,  cos(q1), sin(q1)],
-        [0, -sin(q1), cos(q1)]])
+        parent : ReferenceFrame
+            Reference frame that this reference frame will be rotated relative
+            to.
+        dcm : Matrix, shape(3, 3)
+            Direction cosine matrix that specifies the relative rotation
+            between the two reference frames.
+        rotation matrix : Matrix Rotation Direction
+            The simple matrix rotation relative ``parent`` to ``dcm``.
path: sympy/physics/vector/frame.py
text: @user1:
The same change is required here. Only now the DCM describes the orientation of the child with respect to the parent.

diff_hunk: @@ -7,6 +7,8 @@ Essential Classes
 
 .. autoclass:: sympy.physics.vector.frame.ReferenceFrame
    :members:
+   :exclude-members: orient_explicit
+   :exclude-members: orient_dcm
path: doc/src/modules/physics/vector/api/classes.rst
text: @user1:
You should include `orient_dcm` because we would like people to see it in the online documentation, such that they'll use it.