Removed unnecessary normalization of the created quaternion in from_axis_angle #21916

dhyams · 2021-08-20T19:53:00Z

Removed unnecessary normalization of the created quaternion in the from_axis_angle Quaternion method. The quaternion is normalized by construction.

Foregoing these normalizations is sometimes a key operation to be able to successfully simplify complex algebraic output from these routines.

References to other Issues or PRs
None

Brief description of what is fixed or changed
Removed unnecessary normalization of the created quaternion in the from_axis_angle Quaternion method. The quaternion is normalized by construction.

Other comments
Foregoing these normalizations is sometimes a key operation to be able to successfully simplify complex algebraic output from these routines.

Release Notes

NO ENTRY

sympy-bot · 2021-08-20T19:53:02Z

✅

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

No release notes entry will be added for this pull request.

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

Removed unnecessary normalization of the created quaternion in the from_axis_angle Quaternion method. The quaternion is normalized by construction.

Foregoing these normalizations is sometimes a key operation to be able to successfully simplify complex algebraic output from these routines.

References to other Issues or PRs
None

Brief description of what is fixed or changed
Removed unnecessary normalization of the created quaternion in the from_axis_angle Quaternion method. The quaternion is normalized by construction.

Other comments
Foregoing these normalizations is sometimes a key operation to be able to successfully simplify complex algebraic output from these routines.

Release Notes
<!-- BEGIN RELEASE NOTES -->
NO ENTRY
<!-- END RELEASE NOTES -->

oscarbenjamin · 2021-08-20T19:59:58Z

Can you add some test code in sympy/algebras/tests/test_quaternion.py?

dhyams · 2021-08-20T20:12:49Z

This pull request derived from #21902

dhyams · 2021-08-20T20:40:15Z

added unit tests as requested.

oscarbenjamin · 2021-08-20T20:44:22Z

sympy/algebras/tests/test_quaternion.py

@@ -23,6 +24,22 @@ def test_quaternion_construction():
    nc = Symbol('nc', commutative=False)
    raises(ValueError, lambda: Quaternion(w, x, nc, z))

+@pytest.mark.parametrize(


SymPy has its own test runner which is sort of a copy of pytest. While it is possible to use pytest with the sympy codebase (I always do) we do need to make sure that the tests run without pytest i.e. using the bin/test script when pytest is not installed.

dhyams · 2021-08-20T20:49:14Z

Oh, so is there not a way to do a parametrize with this mechanism as in pytest?

oscarbenjamin · 2021-08-20T20:55:51Z

Oh, so is there not a way to do a parametrize with this mechanism as in pytest?

No, there isn't: #15497 (CC @asmeurer)

dhyams · 2021-08-20T20:56:15Z

unit tests updated to not use pytest.parametrize.

github-actions · 2021-08-20T20:58:26Z

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
     [ed9a550f]       [317c284b]
     <sympy-1.8^0>                 
-         981±7ms          132±1ms     0.13  dsolve.TimeDsolve01.time_dsolve
-      7.51±0.01s       4.03±0.05s     0.54  integrate.TimeIntegrationRisch02.time_doit(100)
-      7.71±0.02s       3.92±0.02s     0.51  integrate.TimeIntegrationRisch02.time_doit_risch(100)
-      63.2±0.9μs       26.2±0.7μs     0.41  matrices.TimeDiagonalEigenvals.time_eigenvals
-        83.1±4μs       51.2±0.8μs     0.62  matrices.TimeMatrixGetItem.time_ImmutableSparseMatrix_getitem
-        79.9±4μs       49.1±0.9μs     0.61  matrices.TimeMatrixGetItem.time_MutableSparseMatrix_getitem
-        76.4±2μs         47.2±2μs     0.62  solve.TimeMatrixArithmetic.time_dense_add(10, 0)
+      10.4±0.2μs       16.8±0.5μs     1.62  solve.TimeMatrixArithmetic.time_dense_add(3, 0)
+      12.1±0.2μs       27.3±0.4μs     2.26  solve.TimeMatrixArithmetic.time_dense_add(3, 5)
+      18.2±0.4μs       35.0±0.6μs     1.93  solve.TimeMatrixArithmetic.time_dense_add(4, 5)
+      34.2±0.5μs       56.8±0.5μs     1.66  solve.TimeMatrixArithmetic.time_dense_add(6, 5)
-     1.15±0.03ms          213±3μs     0.19  solve.TimeMatrixArithmetic.time_dense_multiply(10, 0)
-      41.4±0.5μs       22.1±0.3μs     0.54  solve.TimeMatrixArithmetic.time_dense_multiply(3, 0)
+        67.0±1μs          117±1μs     1.75  solve.TimeMatrixArithmetic.time_dense_multiply(3, 5)
-        92.5±3μs       28.1±0.4μs     0.30  solve.TimeMatrixArithmetic.time_dense_multiply(4, 0)
-        266±10μs       59.3±0.9μs     0.22  solve.TimeMatrixArithmetic.time_dense_multiply(6, 0)
-      78.6±0.8μs         37.8±1μs     0.48  solve.TimeMatrixOperations.time_det(3, 0)
-         159±9μs         89.5±1μs     0.56  solve.TimeMatrixOperations.time_det(3, 2)
-        163±10μs       82.5±0.9μs     0.51  solve.TimeMatrixOperations.time_det(3, 5)
-        81.6±2μs       37.8±0.9μs     0.46  solve.TimeMatrixOperations.time_det_bareiss(3, 0)
-         163±4μs         90.7±1μs     0.56  solve.TimeMatrixOperations.time_det_bareiss(3, 2)
-        151±10μs         82.0±1μs     0.54  solve.TimeMatrixOperations.time_det_bareiss(3, 5)
-        88.1±2μs         38.6±1μs     0.44  solve.TimeMatrixOperations.time_det_berkowitz(3, 0)
-         161±6μs         92.1±3μs     0.57  solve.TimeMatrixOperations.time_det_berkowitz(3, 2)
-         156±5μs         84.0±1μs     0.54  solve.TimeMatrixOperations.time_det_berkowitz(3, 5)
+        677±30μs      1.13±0.05ms     1.67  solve.TimeMatrixOperations.time_det_berkowitz(4, 0)
+        856±20μs      1.67±0.07ms     1.95  solve.TimeMatrixOperations.time_det_berkowitz(4, 2)
+        871±10μs      1.70±0.04ms     1.96  solve.TimeMatrixOperations.time_det_berkowitz(4, 5)
+         282±8μs          432±5μs     1.53  solve.TimeMatrixOperations.time_rank(3, 0)
+         415±8μs         629±20μs     1.52  solve.TimeMatrixOperations.time_rank(4, 0)
+         112±3μs          178±3μs     1.59  solve.TimeMatrixOperations.time_rref(3, 0)
-      4.61±0.2ms      2.78±0.09ms     0.60  solve.TimeRationalSystem.time_linsolve(10)
-        991±20μs         634±30μs     0.64  solve.TimeRationalSystem.time_linsolve(5)
+     2.78±0.09ms       4.20±0.2ms     1.51  solve.TimeSparseSystem.time_linear_eq_to_matrix(20)
+      5.51±0.2ms       8.65±0.4ms     1.57  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).

sympy/algebras/tests/test_quaternion.py

oscarbenjamin · 2021-08-20T21:43:02Z

This statement implies that these changes have some impact:

Foregoing these normalizations is sometimes a key operation to be able to successfully simplify complex algebraic output from these routines.

What impact is that? (Is something better or faster?)

dhyams · 2021-08-21T00:16:03Z

Yes it is:

better. Does not inject unneeded algebraic expressions into the result that are hard to simplify, or sympy fails to simplify, later. This is how I ran across the problem in the first place.
faster; there is no call to normalize() at all, because it's not needed.

This change is something like finding the following line of code:

a = b / 2 * 2

and simplifying it to

a = b

I will put in a good faith effort to create a case that it helps for, but the problem is that my own example is so complex it would not make for a very good unit test.

oscarbenjamin · 2021-08-21T00:24:30Z

better. Does not inject unneeded algebraic expressions into the result that are hard to simplify, or sympy fails to simplify, later. This is how I ran across the problem in the first place.

That suggests that a test could be written that would demonstrate the betterness :)

oscarbenjamin · 2021-08-21T15:00:39Z

I will put in a good faith effort to create a case that it helps for, but the problem is that my own example is so complex it would not make for a very good unit test.

I think I see the problem now. The normalize method calls norm which calls trigsimp. So in simple examples the normalization is simplified away to dividing by 1. The issue arises when trigsimp is unable to simplify the norm to one. By definition then any test for this would actually be testing the failure of trigsimp.

Perhaps it would be good to open an issue about trigsimp for the complicated example then.

An example where norm does not simplify is:

In [23]: Quaternion.from_axis_angle((1, 2, 3), asin(4))
Out[23]: cos(asin(4)/2) + sqrt(14)*sin(asin(4)/2)/14*i + sqrt(14)*sin(asin(4)/2)/7*j + 3*sqrt(14)*sin(asin(4)/2)/14*k

In [24]: Quaternion.from_axis_angle((1, 2, 3), asin(4)).norm()
Out[24]: 
    _______________________
   ╱ 1 - √15⋅ⅈ   1 + √15⋅ⅈ 
  ╱  ───────── + ───────── 
╲╱       2           2

Note that radsimp can simplify this:

In [27]: radsimp(Quaternion.from_axis_angle((1, 2, 3), asin(4)).norm())
Out[27]: 1

So perhaps it would actually be good to use radsimp as well. Ideally simplification would not be done automatically in norm though or at least there should be some way to disable it.

dhyams · 2021-08-23T15:39:50Z

I can't tell in the above comment that you agree that this PR should be merged. I think it should, as there is no need to call normalize() to begin with.

oscarbenjamin · 2021-08-23T16:58:48Z

I can't tell in the above comment that you agree that this PR should be merged

We were discussing coming up with a test for the changed behaviour. You said you weren't sure that you could so I found this case:

In [3]: Quaternion.from_axis_angle((1, 2, 3), asin(4))
Out[3]: 
cos(asin(4)/2)/sqrt((1 - sqrt(15)*I)/2 + (1 + sqrt(15)*I)/2) + sqrt(14)*sin(asin(4)/2)/(14*sqrt((1 - sqrt(15)*I)/2 + (1 + sqrt(15)*I)/2))*i + sqrt(14)*sin(a
sin(4)/2)/(7*sqrt((1 - sqrt(15)*I)/2 + (1 + sqrt(15)*I)/2))*j + 3*sqrt(14)*sin(asin(4)/2)/(14*sqrt((1 - sqrt(15)*I)/2 + (1 + sqrt(15)*I)/2))*k

With the PR that gives:

In [1]: Quaternion.from_axis_angle((1, 2, 3), asin(4))
Out[1]: cos(asin(4)/2) + sqrt(14)*sin(asin(4)/2)/14*i + sqrt(14)*sin(asin(4)/2)/7*j + 3*sqrt(14)*sin(asin(4)/2)/14*k

dhyams · 2021-08-23T18:26:37Z

Oh, I see, forgive me for being dense. Thanks!

oscarbenjamin · 2021-08-23T20:41:11Z

Okay this looks good.

CI has now failed because it has now changed to require updates to the authors file:
https://groups.google.com/g/sympy/c/3DXOKgoeX24

You need to merge or rebase with latest sympy master and then run bin/mailmap_update.py and bin/authors_update.py. Probably the following line should be added to .mailmap:

Daniel Hyams <daniel.hyams@peopletec.com> dhyams <dhyams@gmail.com>

oscarbenjamin · 2021-08-24T00:45:26Z

I think there also needs to be a line like:

Daniel Hyams <dhyams@gmail.com> Daniel Hyams <daniel.hyams@peopletec.com>

This is because all commits need to map to a unique name/email address.

oscarbenjamin · 2021-08-26T15:06:37Z

sympy/algebras/tests/test_quaternion.py

    raises(ValueError, lambda: Quaternion(w, x, nc, z))


+
+def test_quaternion_axis_angle():


There should always be two blank lines between top-level items like functions at module scope.

oscarbenjamin · 2021-08-26T15:07:19Z

Apart from line spacing this looks good to me

dhyams · 2021-08-26T15:12:27Z

Trying to get the mailmap right is really, very frustrating.

dhyams · 2021-08-27T00:38:56Z

I don't know why it is failing code-quality. About to give up on this PR.

oscarbenjamin · 2021-08-27T08:33:50Z

I'm away right now but I can fix it when I get back.

…

On Fri, 27 Aug 2021, 01:39 dhyams, ***@***.***> wrote: I don't know why it is failing code-quality. About to give up on this PR.

…om_axis_angle Quaternion method. The quaternion is normalized by construction. Foregoing these normalizations is sometimes a key operation to be able to successfully simplify complex algebraic output from these routines.

…simplication advantage

oscarbenjamin · 2021-08-30T16:55:20Z

Rebasing this I get the merge conflict:

<<<<<<< HEAD
Kshitij <kshitijparwani.mat18@itbhu.ac.in> Kshitij Parwani <44468674+P-Kshitij@users.noreply.github.com>
=======
Daniel Hyams <dhyams@gmail.com> dhyams <dhyams@gmail.com>
>>>>>>> updated mailmap

This is what was discussed on the mailing list:
https://groups.google.com/g/sympy/c/3DXOKgoeX24/m/1eDTik55BAAJ

The problem is that new names have to go at the end of the AUTHORS file so as soon as one new contributor PR is merged any that were opened before have to be rebased over.

oscarbenjamin · 2021-08-30T17:50:18Z

Oh, I didn't realise the other problem which is that the number of authors is listed explicitly:

diff --git a/AUTHORS b/AUTHORS
index c0ff31c722..0eed4cb71a 100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -4,7 +4,7 @@ those who explicitly didn't want to be mentioned. People with a * next
 to their names are not found in the metadata of the git history. This
 file is generated automatically by running `./bin/authors_update.py`.
 
-There are a total of 1092 authors.
+There are a total of 1093 authors.
 
 Ondřej Čertík <ondrej@certik.cz>
 Fabian Pedregosa <fabian@fseoane.net>

I've added a line to show the diff when the script fails. Just checking that works and then I'll fix the AUTHORS file.

oscarbenjamin · 2021-08-30T19:19:00Z

Thanks!

oscarbenjamin reviewed Aug 20, 2021

View reviewed changes

sympy/algebras/tests/test_quaternion.py Show resolved Hide resolved

oscarbenjamin added the algebras label Aug 20, 2021

mgeier approved these changes Aug 21, 2021

View reviewed changes

dhyams force-pushed the remove_normalize_from_quat_from_axis_angle branch from 46f0ca7 to f5ff3ed Compare August 24, 2021 00:38

oscarbenjamin reviewed Aug 26, 2021

View reviewed changes

dhyams force-pushed the remove_normalize_from_quat_from_axis_angle branch 2 times, most recently from 3ac7da6 to 6e85781 Compare August 26, 2021 15:25

dhyams added 2 commits August 30, 2021 17:52

added unit tests for quaternion axis_angle static constructor

0a13a40

added another unit test for Quaternion axis/angle routine that shows …

7cf4942

…simplication advantage

updated mailmap

4596e45

oscarbenjamin force-pushed the remove_normalize_from_quat_from_axis_angle branch from 03eb7ba to 4596e45 Compare August 30, 2021 16:57

maint(ci): show the changes to AUTHORS using git diff

eadb3d8

maint: update AUTHORS

fceccc5

oscarbenjamin enabled auto-merge August 30, 2021 18:24

oscarbenjamin merged commit 345c3d6 into sympy:master Aug 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removed unnecessary normalization of the created quaternion in from_axis_angle #21916

Removed unnecessary normalization of the created quaternion in from_axis_angle #21916

dhyams commented Aug 20, 2021 •

edited

sympy-bot commented Aug 20, 2021 •

edited

oscarbenjamin commented Aug 20, 2021

dhyams commented Aug 20, 2021

dhyams commented Aug 20, 2021

oscarbenjamin Aug 20, 2021

dhyams commented Aug 20, 2021

oscarbenjamin commented Aug 20, 2021

dhyams commented Aug 20, 2021

github-actions bot commented Aug 20, 2021 •

edited

oscarbenjamin commented Aug 20, 2021

dhyams commented Aug 21, 2021

oscarbenjamin commented Aug 21, 2021

oscarbenjamin commented Aug 21, 2021

dhyams commented Aug 23, 2021

oscarbenjamin commented Aug 23, 2021

dhyams commented Aug 23, 2021

oscarbenjamin commented Aug 23, 2021

oscarbenjamin commented Aug 24, 2021

oscarbenjamin Aug 26, 2021

oscarbenjamin commented Aug 26, 2021

dhyams commented Aug 26, 2021

dhyams commented Aug 27, 2021

oscarbenjamin commented Aug 27, 2021 via email

oscarbenjamin commented Aug 30, 2021

oscarbenjamin commented Aug 30, 2021

oscarbenjamin commented Aug 30, 2021

		raises(ValueError, lambda: Quaternion(w, x, nc, z))



		def test_quaternion_axis_angle():

Removed unnecessary normalization of the created quaternion in from_axis_angle #21916

Removed unnecessary normalization of the created quaternion in from_axis_angle #21916

Conversation

dhyams commented Aug 20, 2021 • edited

sympy-bot commented Aug 20, 2021 • edited

oscarbenjamin commented Aug 20, 2021

dhyams commented Aug 20, 2021

dhyams commented Aug 20, 2021

oscarbenjamin Aug 20, 2021

Choose a reason for hiding this comment

dhyams commented Aug 20, 2021

oscarbenjamin commented Aug 20, 2021

dhyams commented Aug 20, 2021

github-actions bot commented Aug 20, 2021 • edited

oscarbenjamin commented Aug 20, 2021

dhyams commented Aug 21, 2021

oscarbenjamin commented Aug 21, 2021

oscarbenjamin commented Aug 21, 2021

dhyams commented Aug 23, 2021

oscarbenjamin commented Aug 23, 2021

dhyams commented Aug 23, 2021

oscarbenjamin commented Aug 23, 2021

oscarbenjamin commented Aug 24, 2021

oscarbenjamin Aug 26, 2021

Choose a reason for hiding this comment

oscarbenjamin commented Aug 26, 2021

dhyams commented Aug 26, 2021

dhyams commented Aug 27, 2021

oscarbenjamin commented Aug 27, 2021 via email

oscarbenjamin commented Aug 30, 2021

oscarbenjamin commented Aug 30, 2021

oscarbenjamin commented Aug 30, 2021

dhyams commented Aug 20, 2021 •

edited

sympy-bot commented Aug 20, 2021 •

edited

github-actions bot commented Aug 20, 2021 •

edited