[MRG] BIDSPath to allow users to turn off entity check, and replace all make_bids_basename with BIDSPath

[adam2392]

PR Description

Closes #510

• Removes make_bids_basename completely per discussion below in favor of BIDSPath.
• Added the check kwarg to BIDSPath.__init__() and BIDSPath.update() functions.
• created a private _convert_str_to_BIDSPath() function to handle the necessary converting from strings to BIDSPath objects.

The next PR

• Change kind -> suffix and add modality in favor of kind.
• Implement fpath functionality to replace get_bids_fname

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

[codecov-commenter]

Codecov Report

Merging #511 into master will increase coverage by 0.07%.
The diff coverage is 88.00%.

@@            Coverage Diff             @@
##           master     #511      +/-   ##
==========================================
+ Coverage   92.74%   92.82%   +0.07%     
==========================================
  Files          14       14              
  Lines        2026     2020       -6     
==========================================
- Hits         1879     1875       -4     
+ Misses        147      145       -2     

Impacted Files	Coverage Δ
mne_bids/read.py	96.01% <57.14%> (+0.49%)	⬆️
mne_bids/utils.py	94.60% <71.42%> (+0.02%)	⬆️
mne_bids/path.py	95.87% <95.83%> (+0.12%)	⬆️
mne_bids/commands/mne_bids_raw_to_bids.py	94.11% <100.00%> (ø)
mne_bids/copyfiles.py	96.96% <100.00%> (ø)
mne_bids/dig.py	90.71% <100.00%> (ø)
mne_bids/write.py	96.73% <100.00%> (-0.01%)	⬇️

---

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 a6891bf...4ac2fbc. Read the comment docs.

[agramfort]

what we do in mne-study-template is that we do it via update that does not check. If you go this route with a check param/attribute it's probably more explicit.

@hoechenberger wdyt?

[hoechenberger]

I think it's good to have the check option (so one can turn it off); however I also think we should add it to the BIDSPath.update() method – and default to check=True.

[hoechenberger]

Also this makes me wonder, maybe we can drop make_bids_basename() altogether if we add check to BIDSPath.__init__()? The check was the only reason to have make_bids_basename() alongside with BIDSPath, right?

[agramfort]

it's true that if we have this check param then there is no difference between BIDSPath init and the make_bids_basename function that can go...

[adam2392]

it's true that if we have this check param then there is no difference between BIDSPath init and the make_bids_basename function that can go...

Do we want a deprecation cycle, or just axe it now?

[hoechenberger]

Do we want a deprecation cycle, or just axe it now?

I believe we're breaking so much in the next release already that we don't need to care about deprecation cycles 😁

But we could also follow a slightly different approach: make BIDSPath private, and rename make_bids_basename to make_bids_path, which for now would only instantiate a BIDSPath but could be extended in the future to do more work… not sure if that makes sense?

[jasmainak]

I like @hoechenberger 's idea

[hoechenberger]

Just to outline the idea in more detail:

1. add check=True kwarg to make_bids_basename(), BIDSPath.__init__(), and BIDSPath.update()
2. rename make_bids_basename() to make_bids_path()
3. do one of the following
   • either make BIDSPath private -> _BIDSPath – this, however, would potentially make things a little odd when users try to look up the documentation, e.g. for .update()?
   • or add an info box to the BIDSPath documentation, stating that it should be instantiated via make_bids_path() – I'm thinking something similar to mne.Covariance. I would prefer this.

This will allow us to be more flexible and potentially change internals in the future without breaking backward-compatibility.

[jasmainak]

on second thoughts whether you rename to BIDSPath or make_bids_path you're breaking backwards compatibility, so not sure if this more complicated approach would help ...

[agramfort]

what bothers me is that make_bids_path will basically call the BIDSPath constructor without doing anything more. I often say that objects are harder to use for newcomers than plain functions but here I am not sure it's worth having a function. Well documenting how to use BIDSPath object which is starting to become the core swiss army knife of mne-bids seems a smarter move my 2c

…

[hoechenberger]

Ok so then I'd like to revise my proposal and I'd say let's drop make_bids_basename and add check=True to BIDSPath :)

[adam2392]

So this is pretty much good to go once CI is green. It is the first installment of the 3-tiered PR, proposed in: #492 (comment)

[agramfort]

@hoechenberger @jasmainak @sappelhoff ok for you?

[agramfort]

fetch the branch and try to remove it to see if necessary

…

[hoechenberger]

fetch the branch and try to remove it to see if necessary

You mean … WORK MYSELF? Daaamn… ok, will do :)

[agramfort]

you can do it I am sure :)

[hoechenberger]

e7e25c8 makes it such that we don't fall back to an empty-room's info['meas_date'] if the file naming scheme violates BIDS. Instead, we now raise an error and advice users to fix their BIDS dataset. This simplifies get_matched_emptyroom() quite a bit.

I'm starting to like not trying to be clever.

[hoechenberger]

Good to go from my end.

[agramfort]

@sappelhoff @jasmainak do you want to have a look?

[sappelhoff]

LGTM :)

[hoechenberger]

Awesome work, @adam2392!

[jasmainak]

Sorry for my silence, I trust that the PR was in good hands ;)