-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Public name for quaternion logarithm #26597
Conversation
✅ Hi, I am the SymPy bot. 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: This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.13. Click here to see the pull request description that was parsed.
Update The release notes on the wiki have been updated. |
Benchmark results from GitHub Actions Lower numbers are good, higher numbers are bad. A ratio less than 1 Significantly changed benchmark results (PR vs master) Significantly changed benchmark results (master vs previous release) | Change | Before [2487dbb5] | After [986893a0] | Ratio | Benchmark (Parameter) |
|----------|----------------------|---------------------|---------|----------------------------------------------------------------------|
| - | 67.2±0.9ms | 44.6±0.2ms | 0.66 | integrate.TimeIntegrationRisch02.time_doit(10) |
| - | 68.1±0.8ms | 43.6±0.4ms | 0.64 | integrate.TimeIntegrationRisch02.time_doit_risch(10) |
| + | 18.5±0.1μs | 29.5±0.3μs | 1.6 | integrate.TimeIntegrationRisch03.time_doit(1) |
| - | 5.33±0.05ms | 2.87±0.02ms | 0.54 | logic.LogicSuite.time_load_file |
| - | 72.2±0.4ms | 28.4±0.08ms | 0.39 | polys.TimeGCD_GaussInt.time_op(1, 'dense') |
| - | 25.7±0.09ms | 16.9±0.04ms | 0.66 | polys.TimeGCD_GaussInt.time_op(1, 'expr') |
| - | 73.1±0.5ms | 28.8±0.09ms | 0.39 | polys.TimeGCD_GaussInt.time_op(1, 'sparse') |
| - | 252±1ms | 125±0.4ms | 0.5 | polys.TimeGCD_GaussInt.time_op(2, 'dense') |
| - | 254±0.8ms | 124±0.1ms | 0.49 | polys.TimeGCD_GaussInt.time_op(2, 'sparse') |
| - | 647±5ms | 374±2ms | 0.58 | polys.TimeGCD_GaussInt.time_op(3, 'dense') |
| - | 651±3ms | 374±5ms | 0.58 | polys.TimeGCD_GaussInt.time_op(3, 'sparse') |
| - | 490±5μs | 288±3μs | 0.59 | polys.TimeGCD_LinearDenseQuadraticGCD.time_op(1, 'dense') |
| - | 1.79±0.01ms | 1.05±0.01ms | 0.59 | polys.TimeGCD_LinearDenseQuadraticGCD.time_op(2, 'dense') |
| - | 5.75±0.04ms | 3.10±0.01ms | 0.54 | polys.TimeGCD_LinearDenseQuadraticGCD.time_op(3, 'dense') |
| - | 442±2μs | 229±1μs | 0.52 | polys.TimeGCD_QuadraticNonMonicGCD.time_op(1, 'dense') |
| - | 1.48±0.01ms | 680±4μs | 0.46 | polys.TimeGCD_QuadraticNonMonicGCD.time_op(2, 'dense') |
| - | 4.80±0.02ms | 1.64±0.01ms | 0.34 | polys.TimeGCD_QuadraticNonMonicGCD.time_op(3, 'dense') |
| - | 375±2μs | 207±1μs | 0.55 | polys.TimeGCD_SparseGCDHighDegree.time_op(1, 'dense') |
| - | 2.44±0.02ms | 1.24±0.01ms | 0.51 | polys.TimeGCD_SparseGCDHighDegree.time_op(3, 'dense') |
| - | 9.81±0.06ms | 4.32±0.01ms | 0.44 | polys.TimeGCD_SparseGCDHighDegree.time_op(5, 'dense') |
| - | 353±3μs | 169±1μs | 0.48 | polys.TimeGCD_SparseNonMonicQuadratic.time_op(1, 'dense') |
| - | 2.52±0.03ms | 888±4μs | 0.35 | polys.TimeGCD_SparseNonMonicQuadratic.time_op(3, 'dense') |
| - | 9.56±0.07ms | 2.65±0.01ms | 0.28 | polys.TimeGCD_SparseNonMonicQuadratic.time_op(5, 'dense') |
| - | 1.02±0.01ms | 426±3μs | 0.42 | polys.TimePREM_LinearDenseQuadraticGCD.time_op(3, 'dense') |
| - | 1.72±0.02ms | 502±1μs | 0.29 | polys.TimePREM_LinearDenseQuadraticGCD.time_op(3, 'sparse') |
| - | 5.87±0.04ms | 1.79±0.01ms | 0.3 | polys.TimePREM_LinearDenseQuadraticGCD.time_op(5, 'dense') |
| - | 8.43±0.02ms | 1.51±0ms | 0.18 | polys.TimePREM_LinearDenseQuadraticGCD.time_op(5, 'sparse') |
| - | 288±3μs | 63.8±0.3μs | 0.22 | polys.TimePREM_QuadraticNonMonicGCD.time_op(1, 'sparse') |
| - | 3.44±0.02ms | 393±4μs | 0.11 | polys.TimePREM_QuadraticNonMonicGCD.time_op(3, 'dense') |
| - | 3.98±0.03ms | 275±1μs | 0.07 | polys.TimePREM_QuadraticNonMonicGCD.time_op(3, 'sparse') |
| - | 7.02±0.04ms | 1.26±0.01ms | 0.18 | polys.TimePREM_QuadraticNonMonicGCD.time_op(5, 'dense') |
| - | 8.60±0.03ms | 827±2μs | 0.1 | polys.TimePREM_QuadraticNonMonicGCD.time_op(5, 'sparse') |
| - | 5.02±0.02ms | 2.96±0ms | 0.59 | polys.TimeSUBRESULTANTS_LinearDenseQuadraticGCD.time_op(2, 'sparse') |
| - | 11.9±0.1ms | 6.54±0.04ms | 0.55 | polys.TimeSUBRESULTANTS_LinearDenseQuadraticGCD.time_op(3, 'dense') |
| - | 22.7±0.3ms | 8.93±0.01ms | 0.39 | polys.TimeSUBRESULTANTS_LinearDenseQuadraticGCD.time_op(3, 'sparse') |
| - | 5.22±0.02ms | 865±4μs | 0.17 | polys.TimeSUBRESULTANTS_QuadraticNonMonicGCD.time_op(1, 'sparse') |
| - | 12.6±0.1ms | 6.89±0.02ms | 0.55 | polys.TimeSUBRESULTANTS_QuadraticNonMonicGCD.time_op(2, 'sparse') |
| - | 102±0.5ms | 25.9±0.2ms | 0.25 | polys.TimeSUBRESULTANTS_QuadraticNonMonicGCD.time_op(3, 'dense') |
| - | 166±0.9ms | 53.7±0.3ms | 0.32 | polys.TimeSUBRESULTANTS_QuadraticNonMonicGCD.time_op(3, 'sparse') |
| - | 171±0.8μs | 112±0.6μs | 0.65 | polys.TimeSUBRESULTANTS_SparseGCDHighDegree.time_op(1, 'dense') |
| - | 406±70μs | 215±1μs | 0.53 | polys.TimeSUBRESULTANTS_SparseGCDHighDegree.time_op(1, 'sparse') |
| - | 4.29±0.03ms | 847±3μs | 0.2 | polys.TimeSUBRESULTANTS_SparseGCDHighDegree.time_op(3, 'dense') |
| - | 5.33±0.1ms | 381±2μs | 0.07 | polys.TimeSUBRESULTANTS_SparseGCDHighDegree.time_op(3, 'sparse') |
| - | 19.7±0.2ms | 2.82±0.01ms | 0.14 | polys.TimeSUBRESULTANTS_SparseGCDHighDegree.time_op(5, 'dense') |
| - | 22.7±0.1ms | 625±2μs | 0.03 | polys.TimeSUBRESULTANTS_SparseGCDHighDegree.time_op(5, 'sparse') |
| - | 479±1μs | 136±2μs | 0.28 | polys.TimeSUBRESULTANTS_SparseNonMonicQuadratic.time_op(1, 'sparse') |
| - | 4.62±0.03ms | 609±3μs | 0.13 | polys.TimeSUBRESULTANTS_SparseNonMonicQuadratic.time_op(3, 'dense') |
| - | 5.28±0.03ms | 139±0.7μs | 0.03 | polys.TimeSUBRESULTANTS_SparseNonMonicQuadratic.time_op(3, 'sparse') |
| - | 13.1±0.2ms | 1.30±0.01ms | 0.1 | polys.TimeSUBRESULTANTS_SparseNonMonicQuadratic.time_op(5, 'dense') |
| - | 13.7±0.08ms | 141±1μs | 0.01 | polys.TimeSUBRESULTANTS_SparseNonMonicQuadratic.time_op(5, 'sparse') |
| - | 135±1μs | 76.1±0.9μs | 0.57 | solve.TimeMatrixOperations.time_rref(3, 0) |
| - | 253±3μs | 89.1±0.2μs | 0.35 | solve.TimeMatrixOperations.time_rref(4, 0) |
| - | 24.1±0.06ms | 10.2±0.03ms | 0.42 | solve.TimeSolveLinSys189x49.time_solve_lin_sys |
| - | 28.4±0.4ms | 15.2±0.06ms | 0.54 | solve.TimeSparseSystem.time_linsolve_Aaug(20) |
| - | 55.2±0.1ms | 24.5±0.1ms | 0.44 | solve.TimeSparseSystem.time_linsolve_Aaug(30) |
| - | 28.0±0.2ms | 15.0±0.05ms | 0.54 | solve.TimeSparseSystem.time_linsolve_Ab(20) |
| - | 54.8±0.3ms | 24.3±0.09ms | 0.44 | solve.TimeSparseSystem.time_linsolve_Ab(30) |
Full benchmark results can be found as artifacts in GitHub Actions |
This change seems reasonable to me. |
I see this was already merged, thank you! But just want to double-check that the maintainers are OK with removing the old method |
I can verify that nothing is broken, because nothing relevent is missing after I search the code It does not 100% guarantee that the change doesn't break anything in theory |
I think he meant more that there isn't other code that is using it. But I'm ok with removing it since the name was private. |
References to other Issues or PRs
Fixes #26552
Brief description of what is fixed or changed
Renamed the quaternion logarithm method from
Quaternion._ln
toQuaternion.log
.Other comments
Previously the quaternion logarithm was called
_ln()
. This was confusing because the quaternion exponential was given a normal "public" nameexp()
. The logarithm is equally important and was documented equally well, but had a "private" name.Docstring updated to mirror that of
exp()
.Renamed to
log()
instead ofln()
because in most math software,log
refers to the natural logarithm.My own motivating interest in quaternion exp/log is the mappings between the Lie group SO(3) and its Lie algebra so(3) when using the unit quaternion double-covering to represent SO(3). In this context, logarithm bases other than e never occur. I would guess this is the most common use of quaternion exp/log, but I am not sure.
Not sure if we want to add a
_ln()
alias to avoid breaking old code? Or if it can be skipped because callers should not rely on methods that start with an underscore? Please advise.Release Notes
_ln
tolog
.