[GENFI] Update genfi patch #912
Conversation
Co-authored-by: Gensollen <[email protected]>
Co-authored-by: Gensollen <[email protected]>
Co-authored-by: Gensollen <[email protected]>
Co-authored-by: Gensollen <[email protected]>
|
The test should break since the CI needs to be updated |
|
new data can be found here |
|
Unit-tests currently crash for the following reason: I changed the content of the They contradict because GENFI acquired a single b0, which has no bvecs or bvals, which is not allowed by the bids-spec. I'm conflicted regarding this addition of dwi to the bidsignore, I'd rather remove it (even though I've added it), and not let GENFI pass the bids-validator, since it is out of spec. |
There was a problem hiding this comment.
Thanks @MatthieuJoulot !
I made a quick first pass without testing it (will do that later today...).
I'm a bit concerned with the changes made to utility functions used across all converters.
Did you make sure your modifications were not breaking anything ?
clinica/iotools/bids_utils.py
Outdated
| @@ -524,6 +524,7 @@ def _write_bidsignore(bids_dir: Union[str, Path]) -> None: | |||
| with open(Path(bids_dir) / ".bidsignore", "w") as f: | |||
| # pet/ is necessary until PET is added to BIDS standard | |||
| f.write("\n".join(["swi/\n"])) | |||
| f.write("\n".join(["dwi/\n"])) | |||
There was a problem hiding this comment.
Why this change ? Isn't this breaking other converters relying on this function ?
clinica/iotools/converters/genfi_to_bids/genfi_to_bids_utils.py
Outdated
Show resolved
Hide resolved
clinica/iotools/converters/genfi_to_bids/genfi_to_bids_utils.py
Outdated
Show resolved
Hide resolved
clinica/iotools/converters/genfi_to_bids/genfi_to_bids_utils.py
Outdated
Show resolved
Hide resolved
| # ].agg("/".join, axis=1), | ||
| ) | ||
| df_sub_ses_run = df_sub_ses_run.assign( | ||
| bids_filename=lambda df: df.bids_filename.apply(lambda x: x.replace("__", "_")) |
There was a problem hiding this comment.
I don't understand what you're doing here. Why would there exist "__" in the file names ?
There was a problem hiding this comment.
because of the task_rest that is aggregated as well. since it only exists for fMRI acquisitions, two underscores are aggregated for other modalities.
There was a problem hiding this comment.
Hmmm, I think we need to discuss this together. It looks very hacky from a reader's point of view...
The merge_imaging_data was always a weak point with a lot of lines and hard-to-follow logic.
We managed to make it reasonably complex the last time, but it seems to be "complexifying" again.
| json.dump(data, json_file, indent=4) | ||
| with open(json_file, "r+") as f: | ||
| if multipart_id is not None: | ||
| data = json.load(f) |
There was a problem hiding this comment.
I don't know why, but this doesn't work. So I changed back to what I had done in the first place, which I does !
It may not be optimal though, so we can look at it together
There was a problem hiding this comment.
OK, we'll take a look at the error you're getting this afternoon.
Co-authored-by: Gensollen <[email protected]>
| json.dump(data, json_file, indent=4) | ||
| with open(json_file, "r+") as f: | ||
| if multipart_id is not None: | ||
| data = json.load(f) |
There was a problem hiding this comment.
OK, we'll take a look at the error you're getting this afternoon.
| # ].agg("/".join, axis=1), | ||
| ) | ||
| df_sub_ses_run = df_sub_ses_run.assign( | ||
| bids_filename=lambda df: df.bids_filename.apply(lambda x: x.replace("__", "_")) |
There was a problem hiding this comment.
Hmmm, I think we need to discuss this together. It looks very hacky from a reader's point of view...
The merge_imaging_data was always a weak point with a lot of lines and hard-to-follow logic.
We managed to make it reasonably complex the last time, but it seems to be "complexifying" again.
| Example | ||
| ------- | ||
| source_id = 'C9ORF001' | ||
| source_ses_id = '11 |
There was a problem hiding this comment.
11 means it is the first session of GENFI 2
There was a problem hiding this comment.
It's much clearer now that you added an example for source, thanks !
|
Alright ! All tests are passing, merging... |
Processing the whole dataset reveals a few more problems. This PR handles these problems.