[MRG] RF: Rework get_matched_empty_room()

[hoechenberger]

PR Description

Closes #419

Merge checklist

Maintainer, please confirm the following before merging:

• All comments resolved
• This is not your own PR
• All CIs are happy
• PR title starts with [MRG]
• whats_new.rst is updated
• PR description includes phrase "closes <#issue-number>"
• Commit history does not contain any merge commits

[hoechenberger]

Except for the missing tests and changelog entries, this should be ready for review. Note that I'm currently not utilizing scans.tsv as originally proposed in #419. I think we should open a separate PR for this and add this feature in all relevant places if we want it – not just for empty-room recordings.

[codecov-commenter]

Codecov Report

Merging #421 into master will not change coverage.
The diff coverage is 0.00%.

@@          Coverage Diff           @@
##           master    #421   +/-   ##
======================================
  Coverage    0.00%   0.00%           
======================================
  Files          12      12           
  Lines        1616    1658   +42     
======================================
- Misses       1616    1658   +42     

Impacted Files	Coverage Δ
mne_bids/read.py	0.00% <0.00%> (ø)
mne_bids/utils.py	0.00% <0.00%> (ø)
mne_bids/write.py	0.00% <ø> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5e3499d...8c6ce43. Read the comment docs.

[jasmainak]

@hoechenberger looks like the CIs are failing. Also can you check of the items in the checklist above once you are ready?

My main comment is about using Pathlib. If we decide to do this, we should do it consistently in the whole repo so it's easier to read the code.

Thanks for fixing this function!

[hoechenberger]

@hoechenberger looks like the CIs are failing. Also can you check of the items in the checklist above once you are ready?

Yes still a bit WIP :)

My main comment is about using Pathlib. If we decide to do this, we should do it consistently in the whole repo so it's easier to read the code.

Yep I agree. Will revert this to os.path / glob.glob for now.

[hoechenberger]

I will move the "move the core of make_bids_basename() to a private function so we can call it internally with some fancy additional arguments" stuff to a separate PR to make the diff of this one smaller

[hoechenberger]

Will rebase this branch once #427 has been merged.

[hoechenberger]

It appears to me that empty-room recordings do not necessarily need to have task-noise set, am I understanding this correctly? https://bids-specification.readthedocs.io/en/stable/04-modality-specific-files/02-magnetoencephalography.html#empty-room-meg-recordings

cc @sappelhoff

[sappelhoff]

mh yes, there is no MUST or REQUIRED, only

TaskName in the *_meg.json file should be set to "noise".

not even a capital SHOULD ☹️

Did you check whether the validator throws a tantrum if you have, e.g. task-BLA?

[hoechenberger]

not even a capital SHOULD ☹️

Yep... somebody SHOULD have put one there LOL

Did you check whether the validator throws a tantrum if you have, e.g. task-BLA?

No I haven't checked. Anyway, we're supporting READING those files now, but we're not allowing users to CREATE them. Which is probably good?

[hoechenberger]

Did you check whether the validator throws a tantrum if you have, e.g. task-BLA?

No I haven't checked. Anyway, we're supporting READING those files now, but we're not allowing users to CREATE them. Which is probably good?

Just checked, and no, it does not complain, neither for a missing task, nor for a non-noise task.

[hoechenberger]

@jasmainak I think I'm done here, are you happy with these changes?

[jasmainak]

@hoechenberger no test to update?

[jasmainak]

also a CI is failing, is it related to this PR?

[hoechenberger]

@hoechenberger no test to update?

I was gonna add some, but it's so painful… the current tests are so convoluted and full of side-effects that I find it extremely frustrating to add to the existing tests. But you're right, will look into this.

also a CI is failing, is it related to this PR?

The GH Actions infrastructure is still WIP for us, so you can ignore this for now :)

[jasmainak]

but it's so painful

@agramfort says "no pain, no gain" :P

[hoechenberger]

I ran int an upstream bug that needs to be fixed first for my tests to work:
mne-tools/mne-python#7816

(Update: Apparently it's not a bug…)

@agramfort says "no pain, no gain" :P

I say, "If there's too much pain, maybe you should pick your brain" :D I'd like to refactor the tests sometime in the future, I don't believe in overly complex & long tests :)

[agramfort]

@hoechenberger you need to rebase.

[hoechenberger]

@agramfort I'm currently being blocked by mne-tools/mne-python#7816 as it prevents me from adding a test which I have already written

[hoechenberger]

Please do not merge yet.

[jasmainak]

We won't merge until you set WIP to MRG :)

weird that the meas_date bug keeps coming back ... I thought we had tests in MNE to deal with that.

[hoechenberger]

This is good to merge.

[agramfort]

besides this minor thing +1 for MRG

[jasmainak]

I know you want to move fast but please never merge your own PR :) Unless it's really a trivial one and there's nobody else to merge.

[hoechenberger]

Oh ok, sorry about that! Thought once positive reviews are in, it doesn't matter. But will follow this direction in the future!