Skip to content
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

Coreg fixes #9928

Merged
merged 4 commits into from
Nov 1, 2021
Merged

Coreg fixes #9928

merged 4 commits into from
Nov 1, 2021

Conversation

bloyl
Copy link
Contributor

@bloyl bloyl commented Oct 30, 2021

Handle two issues I raised in #8833,

  1. support for info['dig'] missing some types of dig points
  2. support loading from instances only containing dig objects.

still needs latest.inc hasn't been released so no need.

Copy link
Member

@agramfort agramfort left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thx @bloyl

does it full test what you adjusted? should we add a test of the Coregistration class using some CTF data?

mne/tests/test_coreg.py Outdated Show resolved Hide resolved
Copy link
Member

@agramfort agramfort left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think CI failures are related

Good enough to go from my end. we should not release without this fix.

thx heaps @bloyl

@larsoner larsoner added this to the 0.24 milestone Nov 1, 2021
@@ -215,10 +220,23 @@ def _set_current_fiducial(self, fid):
def _set_info_file(self, fname):
if fname is None:
return
if not self._check_fif('info', fname):

# info file can be anything supported by read_raw
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On main it can actually be more than this -- it could be an evoked or epochs etc. -- really anything that stored an info in a .fif. This change makes it so that, for FIF, only the raw variants are supported. I'll push a change to fix this just based on a splitext check

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh never mind, I misunderstood this code -- looks like you do handle fif using a read-info-like operation below!

Copy link
Member

@larsoner larsoner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @bloyl !

@larsoner larsoner merged commit 9cbff29 into mne-tools:main Nov 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants