ENH: Allow refitting HPI and dev_head_t #13484
Conversation
for more information, see https://pre-commit.ci
* upstream/main: [pre-commit.ci] pre-commit autoupdate (mne-tools#13482)
larsoner
requested review from
agramfort,
alexrockhill,
dengemann and
drammock
as code owners
|
@drammock I think this one is good to go! |
|
I don't see this one as a blocker for 1.11 -- can be an early 1.12 so bumping milestone |
drammock
left a comment
drammock
left a comment
There was a problem hiding this comment.
just a few comments/suggestions/questions. Didn't look too closely at the tests.
| assert isinstance(desired, Transform), "Both must be Transform or ndarray" | ||
| assert actual["from"] == desired["from"] | ||
| assert actual["to"] == desired["to"] | ||
| actual = actual["trans"] | ||
| desired = desired["trans"] | ||
| assert isinstance(actual, np.ndarray) | ||
| assert isinstance(desired, np.ndarray) | ||
| assert actual.shape == (4, 4) | ||
| assert desired.shape == (4, 4) | ||
| angle, dist = angle_distance_between_rigid( | ||
| actual, desired, angle_units="deg", distance_units="m" | ||
| ) | ||
| assert dist <= dist_tol, ( | ||
| f"{1000 * dist:0.3f} > {1000 * dist_tol:0.3f} mm translation" | ||
| ) | ||
| assert angle <= angle_tol, f"{angle:0.3f} > {angle_tol:0.3f}° rotation" |
There was a problem hiding this comment.
might be good for more of these asserts to have explanatory text for failures
There was a problem hiding this comment.
I thought the pytest default assert output should ideally be good enough. But it turned out:
assert actual.shape == (4, 4)
Didn't say anything. BUT! I discovered:
https://docs.pytest.org/en/stable/how-to/assert.html#assertion-introspection-details
can make it say something like this (if I change it to (3, 4) just to get it to fail, which seems clear enough to me given the traceback
mne/tests/test_chpi.py:976: in test_refit_hpi_locs_problematic
assert_trans_allclose(
E assert (4, 4) == (3, 4)
E
E At index 0 diff: 4 != 3
E Use -v to get more diff
so knowing that it overwrites some of it, I can at least add a little less text. Will push a commit shortly
mne/chpi.py
Outdated
| for the best goodness of fit between digitized coil locations and | ||
| (rigid-transformed) fitted coil locations. | ||
| 4. Subselect coils to use for fitting ``dev_head_t`` based on ``gof_limit``, | ||
| ``dist_limit``, and ``max_use``. |
There was a problem hiding this comment.
| ``dist_limit``, and ``max_use``. | |
| ``dist_limit``, and ``use``. |
or, change param name to max_use, that might be better actually?
There was a problem hiding this comment.
Well it can be a list of coils to use, too, which is why I changed it from max_use (when initially I only allowed setting it as a max count) to use (when I also allowed it to be a list of int)
|
Thanks @drammock I think I addressed the comments! I'll mark for merge-when-green to sneak it in for 1.11. I think it should be good enough for people to try |
2 participants
Sometimes the Neuromag system screws up the initial HPI fitting, which then messes up
dev_head_tand also any subsequent continuous head position estimation. This PR will fix that. Lots of TODOs in the code but I wanted to get it up to have a place to track progress.if proj_op is Noneadditionwhats_newentry