MRG, MAINT: Refactor and unify matched-point fitting #7325
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
agramfort
reviewed
Feb 14, 2020
| mu_p = mean(p, axis=0) # eq 23 | ||
| mu_x = mean(x, axis=0) | ||
| Sigma_px = np.dot(p.T, x) / p.shape[0] - np.outer(mu_p, mu_x) # eq 24 | ||
| # (weighted) centroids |
Member
There was a problem hiding this comment.
Maybe you can use np.average and the accepts weights ?
Member
Author
There was a problem hiding this comment.
This is in jit-ed code, and Numba does not support it. So not worth the speed loss :)
Codecov Report
@@ Coverage Diff @@
## master #7325 +/- ##
=========================================
Coverage ? 89.71%
=========================================
Files ? 450
Lines ? 81353
Branches ? 12932
=========================================
Hits ? 72983
Misses ? 5519
Partials ? 2851 |
Member
Author
|
Still some |
larsoner
changed the title
Closed
larsoner
changed the title
Member
Author
|
Okay this one should be ready to go |
Member
Author
|
Travis failure is just a timeout due to non-BLAS NumPy prerelease wheels |
Member
|
Thx @larsoner |
AdoNunes
pushed a commit
to AdoNunes/mne-python
that referenced
this pull request
Apr 6, 2020
* MAINT: Refactor and unify matched-point fitting * FIX: Casts * FIX: Try again * ENH: Speed up 3D test a tiny bit * FIX: Verbose * DOC: Fix comment [ci skip] * MAINT: From transforms
AdoNunes
pushed a commit
to AdoNunes/mne-python
that referenced
this pull request
Apr 6, 2020
* MAINT: Refactor and unify matched-point fitting * FIX: Casts * FIX: Try again * ENH: Speed up 3D test a tiny bit * FIX: Verbose * DOC: Fix comment [ci skip] * MAINT: From transforms
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When implementing the cHPI changes, I noticed some places that our use of matched-point fitting could be improved. Specifically:
mne coreguses matched point fitting in ICP. This now routes the code to use the analytical solution when possible._quat_to_eulerand_euler_to_quatfrom standard formulas.mne coregcaused me to look back at the ICP ref, then the original ref for the analytical transform, and realize that 1) it can include weights, and 2) it can compute a uniform scale factor, too. So I implemented those, which expanded the use cases formne coreg.mne.io.ctfcomputed some transforms using its own matched point code, and also its own RAS-to-head. These now use transforms frommne.transformsinstead.So there ended up being more + than - in this PR, but we removed more functional code than we added -- a lot of the +` comefrom new tests.