RF: embed_nifti -> embed_dicom_and_nifti_metadata and not invoke it if min_meta #432
Conversation
If I saw it right, the only relevant conditioned on min_meta inside embed_metadata_from_dicoms was embedding taskname from the filename, which is not per se from dicom, so not appropriate there. Another side effect might be to miss prov information if min_meta, but since we would not be running that function and not embedding from dicoms -- I think it is ok
…tadata That function was returning a new nifti constructed from dicoms if nifti file did not exist before. That lead to also different returns in comparison to operation whenever used on an existing nifti file. All conversions to nifti should be done centrally, using dcm2niix (via nipype_convert) and we should avoid possible other tool to create nifti files -- could lead to inconsistencies etc. Now that the function does not produce those files or changes those passed names, there is no need for it to return anything. So it would return None. Test adjusted accordingly to use nipype_convert
…chdir back Side effects from tests, like going to another directory are pain, we should avoid them
Codecov Report
@@ Coverage Diff @@
## master #432 +/- ##
==========================================
- Coverage 74.92% 74.85% -0.07%
==========================================
Files 35 35
Lines 2831 2851 +20
==========================================
+ Hits 2121 2134 +13
- Misses 710 717 +7
Continue to review full report at Codecov.
|
heudiconv/dicoms.py
Outdated
|
|
||
| if bids_info: | ||
| # make nice with python 3 - same behavior? | ||
| meta_info = meta_info.copy() |
There was a problem hiding this comment.
I'm not sure why we make a copy here
| meta_info = meta_info.copy() |
There was a problem hiding this comment.
yep, seems to be no longer necessary -- removing!
| out = json.loads(out) | ||
| # should have created nifti file | ||
| assert op.exists('nifti.nii') | ||
| # 1) nifti does not exist -- no longer supported |
There was a problem hiding this comment.
we should test that the NotImplementedError is being hit
There was a problem hiding this comment.
easy enough and wortwhile indeed - done in 4fd4391
| json.dump(meta_info, fp, indent=3, sort_keys=True) | ||
|
|
||
| return niftifile, infofile | ||
| save_json(infofile, meta_info) |
There was a problem hiding this comment.
should we keep the same indentation here?
There was a problem hiding this comment.
I was on an edge for this one, and decided that we better do not delay fixing this inconsistency (which we should have done long ago). Especially now since in case of min_meta, file would be saved with our "standard" indentation (of 4), thus differ from this one. So, I decided to stay consistent and just use save_json with defaults...
but now you made me look at how we use save_json and it seems that we prefer indent=2 in many places:
$> git grep -A20 'save_json(' | grep -e 'save_json' -e 'indent'
heudiconv/bids.py: save_json(descriptor,
heudiconv/bids.py: save_json(task_file, fields, indent=2, sort_keys=True, pretty=True)
heudiconv/bids.py: save_json(jsonfile, json_, indent=2)
heudiconv/bids.py: save_json(json_phasediffname, json_, indent=2)
heudiconv/bids.py: save_json(participants_json,
heudiconv/bids.py- indent=2)
heudiconv/bids.py: save_json(scans_json,
heudiconv/bids.py- indent=2)
heudiconv/convert.py: save_json(filegroup_file, filegroup)
heudiconv/convert.py: save_json(infofile, meta_info)
heudiconv/dicoms.py: save_json(infofile, meta_info)
heudiconv/tests/test_utils.py: save_json(valid_json_file, vcontent)
heudiconv/utils.py:def save_json(filename, data, indent=4, sort_keys=True, pretty=False):
heudiconv/utils.py- indent : int, optional
heudiconv/utils.py- data, sort_keys=sort_keys, indent=indent)
As a separate PR I will propose making default indent to be 2 and thus removing any "ad-hoc" indent use
|
Ok, let's proceed |
Follow up to #420
embed_dicom_and_nifti_metadatawill not produce nifti file if that one doesn't exist yet . This functionality (hopefully) was not used for a while, and we should rely on dcm2niix for all such conversionssave_jsoninstead of ad-hocjson.dumpcall withindent=3. So with this PR we would end up changing .json files formatting a bit, which could complicate regression testing, but I think consistency is worth it@assert_cwd_unchangednow can be used to assure that tests do not jump to some other directories, or force their return (note that if directory has symlinks in the path -- we would return to dereferenced path)