[MRG] Updating make_bids_basename to return BIDSPath, allowing users to dynamically alter the bids file name #446
Conversation
adam2392
changed the title
|
@jasmainak can you do just a skim and lmk if this is in line w/ where you were thinking? I haven't done too many changes yet, but wasn't sure what the best way to integrate BIDSPath after we discussed in #257. |
|
@adam2392 I think it might be easier if you cherry-pick my commits, then try to make the tests pass. |
|
Can you also change the codebase to use this everywhere? Show me red lines :) |
I guess my concern is right now, Before making the change throughout the codebase, I figured it would be good to decide which path to take? |
Codecov Report
@@ Coverage Diff @@
## master #446 +/- ##
==========================================
+ Coverage 93.36% 93.49% +0.12%
==========================================
Files 12 12
Lines 1674 1721 +47
==========================================
+ Hits 1563 1609 +46
- Misses 111 112 +1
Continue to review full report at Codecov.
|
isn't this the desired behavior? |
Yes true, I was just getting worried about what happens when downstream functions do things tho that assume it's a string, but turns out this was mitigated by just overriding some of the magic methods for BIDSPath. |
Okay I am refactoring to make it operate as an object instead of dict. Regarding how it is constructed, do we want to keep the function |
|
I would keep |
|
@adam2392 your challenge is to get more red lines than green lines. Can it be done? :) You can also update/simplify code in tests I believe (don't change logic though) |
This turned out to be quite a challenge imo! So the added class turns out to be ~170 lines, so... I think I did neutral :p. I would hope that the changes in a few of the lines are convincing enough for you and @agramfort that it simplifies user code(?) |
|
$ python -c "import this" | grep "only"
There should be one-- and preferably only one --obvious way to do it.
don't make API a mess to save 4 key strokes
… |
|
I've updated the comments above and left some notes to trace back to each decision. Everything you have commented on, I tried addressing and summarizing above. However, I am not confident in the integration of 'emptyroom' subjects, so hoping to get your input there. According to codecov report, most lines are covered, with the exception of |
| reader = {'.con': io.read_raw_kit, '.sqd': io.read_raw_kit, | ||
| '.fif': io.read_raw_fif, '.pdf': io.read_raw_bti, | ||
| '.ds': io.read_raw_ctf, '.vhdr': io.read_raw_brainvision, | ||
| '.edf': io.read_raw_edf, '.bdf': io.read_raw_bdf, | ||
| '.set': io.read_raw_eeglab} |
There was a problem hiding this comment.
Needed to move this here to prevent circular imports.
| """ | ||
| return deepcopy(self) | ||
|
|
||
| def get_bids_fname(self, kind=None, bids_root=None, extension=None): |
There was a problem hiding this comment.
Essentially copied version of _make_bids_fname.
There was a problem hiding this comment.
So, is BIDSPath by default not the full path? Why is that?
There was a problem hiding this comment.
If the BIDSPath object is the full path, then it's not really the bids_basename anymore, which doesn't seem required by the API right now though?
The internals of read_raw_bids and write_raw_bids doesn't require the full fname. This function operates like _make_bids_fname and can "infer" the kind and the extension by searching down the bids_root.
Otherwise, we can alter this back to _make_bids_fname, which accepts the bids_basename as a BIDSPath object instead of a string and don't have it as part of the BIDSPath.
We can either:
- Modify this to incorporate a full path at all times? Not sure what this should look like though.
- Turn this into a private function?
- Add some further edits to this function, but keeping the header as is?
- Modify it back to
_make_bids_fname?
There was a problem hiding this comment.
@adam2392 let's discuss this in a new issue. We have spent too long on this PR. Let's merge this.
There was a problem hiding this comment.
Not really sure how to articulate the issue you're thinking of... :| can you make it?
There was a problem hiding this comment.
reading this again I think @jasmainak has a point here. The fact that get_bids_fname returns BIDSPath and not a str is a code smell. It suggests that BIDSPath can be informed by bids_root and kind.
wdyt?
There was a problem hiding this comment.
Ah yeah I see; it does seem like it could be confusing. So would the suggested fix be to:
- make it return a str?
or - add bids_root/kind to BIDSPath explicitly?
| suffix=suffix) | ||
|
|
||
|
|
||
| def _get_bids_fname_from_filesystem(*, bids_basename, bids_root, sub, ses, |
There was a problem hiding this comment.
Needed to move this here to prevent circular imports.
| return bids_fname | ||
|
|
||
|
|
||
| def _infer_kind(*, bids_basename, bids_root, sub, ses): |
There was a problem hiding this comment.
Needed to move here to prevent circular imports.
| return bids_fname | ||
|
|
||
|
|
||
| def _make_bids_fname(bids_basename, bids_root=None, kind=None, extension=None, |
There was a problem hiding this comment.
moved whole function into BIDSPath.
| return kinds[0] | ||
|
|
||
|
|
||
| def _get_bids_fname_from_filesystem(*, bids_basename, bids_root, sub, ses, |
| @@ -264,118 +258,6 @@ def _handle_channels_reading(channels_fname, bids_fname, raw): | |||
| return raw | |||
|
|
|||
|
|
|||
| def _infer_kind(*, bids_basename, bids_root, sub, ses): | |||
| _gen_bids_basename, | ||
| _estimate_line_freq, _get_kinds_for_sub) | ||
|
|
||
| reader = {'.con': io.read_raw_kit, '.sqd': io.read_raw_kit, |
|
this handling of emptyroom and the _gen_bids_basename was really a code smell. I pushed a changed here. Let me know what you think. Basically I made so that BIDSPath does not do any check and you only get checks with make_bids_basename keeping BIDSPath as an internal tool |
|
@adam2392 can you check what I did and update what's new? than good to go from my end |
adam2392
changed the title
|
Thanks heaps @adam2392 for the PR! |
|
what would be the pros and cons of either options?
… |
`get_head_mri_trans()` in `master` tries to infer the filename extension from a BIDS basename; however, the basename, as is created inside that function, doesn't have a suffix. Correctly, the extension has to be inferred from `bids_fname`. This is what we used to do before mne-toolsGH-446 got merged. This commit restores the old behavior. (I realized something was going wrong when MNE-BIDS started looking for some .pdf files, while I only have .fif. So beyond what I'm fixing here, there seems to be another bug in `parse_ext()`, causing it to report `.pdf` when really it shouldn't.)
`get_head_mri_trans()` in `master` tries to infer the filename extension from a BIDS basename; however, the basename, as is created inside that function, doesn't have a suffix. Correctly, the extension has to be inferred from `bids_fname`. This is what we used to do before GH-446 got merged. This commit restores the old behavior. (I realized something was going wrong when MNE-BIDS started looking for some .pdf files, while I only have .fif. So beyond what I'm fixing here, there seems to be another bug in `parse_ext()`, causing it to report `.pdf` when really it shouldn't.)
PR Description
Closes: #445
Closes: #257
This is a draft to let maintainers see what I mean. Once comments are suggested and made, will iterate. The goal of this PR is to enable a bids filename created via
make_bids_basenameto be dynamically altered based on its BIDS entity without callingmake_bids_basenameagain and again.In order to do so,
make_bids_basenamenow returns aBIDSPathobject started by @jasmainak. In order to make the PR work w/o changing a lot of code, it was ideal to giveBIDSPathas many string functionality as possible, because at the end of the day it is just a structured string, stored as a dictionary.I override various magic methods. The
__str__and__repr__ofBIDSPathis slightly borrowed from pathlib.Path. Some "magic" method behavior is listed here:__eq__: makes BIDSPath as a str__fspath__: same as pathlib.Path__bytes__: same as pathlib.Pathcopy(): returns deepcopy of selfupdate: updates multiple entities at once, allowing aliases for subject, session, recording, acquisition, processingentities: returns an ordered dictionary of all the BIDS entitites.TODO:
make_bids_basenameoutputs a string.Currently, I have a flagas_str=Trueto make tests pass, but idk if that's the best solution.Merge checklist
Maintainer, please confirm the following before merging: