Implemented Givens rotation #24535

evbernardes · 2023-01-16T20:25:21Z

References to other Issues or PRs

As discussed in #24529.

Brief description of what is fixed or changed

Implemented a method giving the Givens rotation, a generalization of rotation matrices in any number of dimensions.

Replace implementation in other rotation matrix methods by calls to this generalized function.

Release Notes

matrices
- Implemented rot_givens to compute givens rotation matrix.

sympy-bot · 2023-01-16T20:25:23Z

✅

Hi, I am the SymPy bot (v169). I'm here to help you write a release notes entry. Please read the guide on how to write release notes.

Your release notes are in good order.

Here is what the release notes will look like:

matrices
- Implemented rot_givens to compute givens rotation matrix. (#24535 by @evbernardes)

This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.12.

Click here to see the pull request description that was parsed.

#### References to other Issues or PRs
As discussed in #24529.

#### Brief description of what is fixed or changed
Implemented a method giving the Givens rotation, a generalization of rotation matrices in any number of dimensions.

Replace implementation in other rotation matrix methods by calls to this generalized function.

#### Release Notes

<!-- BEGIN RELEASE NOTES -->
* matrices
  * Implemented `rot_givens` to compute givens rotation matrix.
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

evbernardes · 2023-01-16T21:19:29Z

@sylee957 Added the cross-references, implemented rot_givens and reimplemented the other ones as calls to rot_givens.

Not sure which other tests to implement, but there were already some tests before that I had introduced (comparing to the results of Quaternion.to_rotation_matrix) and they are still passing, so at least for 3D rotations they work as expected (with negated angle for rot_axis2, as expected from the article on Wikipedia.

sylee957 · 2023-01-16T21:50:47Z

sympy/matrices/dense.py

+    M[i-1, i-1] = c
+    M[j-1, j-1] = c
+    M[i-1, j-1] = s
+    M[j-1, i-1] = -s


I think that 0 to n-1 indexing is more preferable because of having less complicated formula for indexing

Alright, I can change that tomorrow.

Isn't it better to keep it consistent with axis1, axis2 and axis3 though?

No, those methods are pretty much nominal, they have only a finite numbers of names, so they can be left as is.
However a program that generates infinite numbers of notations is more general and can be difficult to change in the future.
It is better to be consistent with python indexing, or sympy matrix indexing itself, or there would be other counter arguments.

Fair, just changed it!

sylee957 · 2023-01-16T21:54:30Z

sympy/matrices/dense.py

+    M = eye(dim)
+    if i < j:
+        i, j = j, i
+        theta = -theta


Is it necessary to calculate this formula?

In the article they said "for i > j", which is why I put it.

But now that I think about it, I think this has no effect indeed.

I removed it and then the calls for rot_givens had to be changed.

It all still works, but I'm not sure which is the correct convention, should be better understood before merging in order not to create something that we can't change later on...

I think that the previous implementation can be wrong because you attempt to change theta but values of s = sin(theta) is already computed above and ignored.

Oops you're right!

sylee957 · 2023-01-16T21:55:59Z

sympy/matrices/dense.py

+        R  = \begin{bmatrix}
+                \cos(\theta) & \sin(\theta) & 0\\
+                -\sin(\theta) & \cos(\theta) & 0\\
+                0 & 0 & 1
+            \end{bmatrix}


Math formula should also better be aligned for indentation
and have constant spacing.

I tried align all of the &, tell me if it's okay!

evbernardes · 2023-01-16T22:23:55Z

bin/test_sphinx.sh and make html are running locally, but it fails in one of the tests here.

Does anyone know why?

oscarbenjamin · 2023-01-16T23:19:52Z

bin/test_sphinx.sh and make html are running locally, but it fails in one of the tests here.

I've restarted it to see if is passes

sylee957 · 2023-01-17T16:25:00Z

I think that the sphinx errors in https://github.com/sympy/sympy/actions/runs/3938511339/jobs/6737742971 should be fixed in some way though.

evbernardes · 2023-01-17T16:29:47Z

I think that the sphinx errors in https://github.com/sympy/sympy/actions/runs/3938511339/jobs/6737742971 should be fixed in some way though.

Agreed, I'll try to investigate it. Do you have any idea of what might be causing it?

sylee957 · 2023-01-18T06:21:38Z

It looks like align may have some problems. I don't think that it had been used by other parts of sympy, so although it can be a bug in sphinx, I'd not recommend using it and find other alternatives to typeset.

github-actions · 2023-01-18T11:51:19Z

Benchmark results from GitHub Actions

Lower numbers are good, higher numbers are bad. A ratio less than 1
means a speed up and greater than 1 means a slowdown. Green lines
beginning with + are slowdowns (the PR is slower then master or
master is slower than the previous release). Red lines beginning
with - are speedups.

Significantly changed benchmark results (PR vs master)

Significantly changed benchmark results (master vs previous release)

       before           after         ratio
     [41d90958]       [3450b757]
     <sympy-1.11.1^0>                 
-     1.14±0.01ms          723±7μs     0.63  solve.TimeSparseSystem.time_linear_eq_to_matrix(10)
-     3.40±0.06ms      1.36±0.03ms     0.40  solve.TimeSparseSystem.time_linear_eq_to_matrix(20)
-      6.75±0.1ms      1.98±0.06ms     0.29  solve.TimeSparseSystem.time_linear_eq_to_matrix(30)

Full benchmark results can be found as artifacts in GitHub Actions
(click on checks at the top of the PR).

evbernardes · 2023-01-18T11:55:32Z

It looks like align may have some problems. I don't think that it had been used by other parts of sympy, so although it can be a bug in sphinx, I'd not recommend using it and find other alternatives to typeset.

Apparently you were right! All checks are passing now.

sympy/matrices/dense.py

Co-authored-by: (Lazard) S.Y. Lee <[email protected]>

evbernardes added 3 commits January 16, 2023 20:52

added cross references between rot methods

a870e2d

implemented givens rotation

a37044b

minor edit

7b124c7

evbernardes added 3 commits January 16, 2023 21:42

fixed docstring

75db4d0

Docstring bug again

77205c8

Inline math mode docs

e92f822

evbernardes marked this pull request as ready for review January 16, 2023 21:16

minor edit

d996516

sylee957 reviewed Jan 16, 2023

View reviewed changes

oscarbenjamin added the matrices label Jan 16, 2023

evbernardes added 3 commits January 17, 2023 10:59

Re-aligned rot docstring math

b6d000f

Simplified rot_givens

dd63067

Fixed docstring test

b20fe48

minor edit in comment

b919867

This comment was marked as resolved.

Sign in to view

evbernardes added 5 commits January 18, 2023 09:42

Removed align from docstring

bac2555

Minor edit

bb079d1

minor edit

fba9211

Removed submatrix from docstring

22bba0e

Fixed ident

0fe1f21

Changed inputs to rot_givens

247059b

sylee957 reviewed Jan 18, 2023

View reviewed changes

sympy/matrices/dense.py Outdated Show resolved Hide resolved

Fixed typo

2cf6870

Co-authored-by: (Lazard) S.Y. Lee <[email protected]>

sylee957 enabled auto-merge January 18, 2023 19:40

sylee957 merged commit 8c8ad9d into sympy:master Jan 18, 2023

evbernardes deleted the rot_givens branch January 18, 2023 20:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented Givens rotation #24535

Implemented Givens rotation #24535

evbernardes commented Jan 16, 2023 •

edited by sylee957

Loading

sympy-bot commented Jan 16, 2023 •

edited

Loading

evbernardes commented Jan 16, 2023 •

edited

Loading

sylee957 Jan 16, 2023 •

edited

Loading

evbernardes Jan 16, 2023

sylee957 Jan 18, 2023

evbernardes Jan 18, 2023

sylee957 Jan 16, 2023

evbernardes Jan 16, 2023

evbernardes Jan 17, 2023

sylee957 Jan 17, 2023

evbernardes Jan 17, 2023

sylee957 Jan 16, 2023

evbernardes Jan 17, 2023

evbernardes commented Jan 16, 2023

oscarbenjamin commented Jan 16, 2023

sylee957 commented Jan 17, 2023

evbernardes commented Jan 17, 2023

This comment was marked as resolved.

sylee957 commented Jan 18, 2023 •

edited

Loading

github-actions bot commented Jan 18, 2023 •

edited

Loading

evbernardes commented Jan 18, 2023

Implemented Givens rotation #24535

Implemented Givens rotation #24535

Conversation

evbernardes commented Jan 16, 2023 • edited by sylee957 Loading

References to other Issues or PRs

Brief description of what is fixed or changed

Release Notes

sympy-bot commented Jan 16, 2023 • edited Loading

evbernardes commented Jan 16, 2023 • edited Loading

sylee957 Jan 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evbernardes commented Jan 16, 2023

oscarbenjamin commented Jan 16, 2023

sylee957 commented Jan 17, 2023

evbernardes commented Jan 17, 2023

This comment was marked as resolved.

sylee957 commented Jan 18, 2023 • edited Loading

github-actions bot commented Jan 18, 2023 • edited Loading

evbernardes commented Jan 18, 2023

evbernardes commented Jan 16, 2023 •

edited by sylee957

Loading

sympy-bot commented Jan 16, 2023 •

edited

Loading

evbernardes commented Jan 16, 2023 •

edited

Loading

sylee957 Jan 16, 2023 •

edited

Loading

sylee957 commented Jan 18, 2023 •

edited

Loading

github-actions bot commented Jan 18, 2023 •

edited

Loading