RF+BF: make code a bit more pythonic, and change comparison > 1 not >=1 for deciding multi-echo #339
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…= 1 for deciding multi-echo
Codecov Report
@@ Coverage Diff @@
## master #339 +/- ##
==========================================
- Coverage 74.24% 74.19% -0.05%
==========================================
Files 35 35
Lines 2671 2666 -5
==========================================
- Hits 1983 1978 -5
Misses 688 688
Continue to review full report at Codecov.
|
yarikoptic
commented
May 15, 2019
| # To see if the echo times are the same, convert it to a set and see if | ||
| # only one remains: | ||
| is_multiecho = len(echo_times) >= 1 if echo_times else False | ||
| is_multiecho = len(echo_times) > 1 |
There was a problem hiding this comment.
FWIW:
original condition for >=1 was added in 266de4a changing previous, IMHO also incorrect test:
# To see if the echo times are the same, convert it to a set and see if
- # only one remains:
- multiecho = False
- if echo_times:
- multiecho = len(set(echo_times)) == 1
+ # only one remains:
+ is_multiecho = len(echo_times) >= 1 if echo_times else Falsewhich gives me some more confidence that my change is correct or my understanding is completely flawed ;)
|
attn @pvelasco - I am here adjusting the logic for deciding if sequence multi-echo, please review |
|
Hi @yarikoptic, |
yarikoptic
added a commit
that referenced
this pull request
Dec 16, 2019
This is largely a bug fix. Metadata and order of `_key-value` fields in BIDS could change from the result of converting using previous versions, thus minor version boost. 14 people contributed to this release -- thanks [everyone](https://github.com/nipy/heudiconv/graphs/contributors)! Enhancement - Use [etelemetry](https://pypi.org/project/etelemetry) to inform about most recent available version of heudiconv. Please set `NO_ET` environment variable if you want to disable it ([#369][]) - BIDS: - `--bids` flag became an option. It can (optionally) accept `notop` value to avoid creation of top level files (`CHANGES`, `dataset_description.json`, etc) as a workaround during parallel execution to avoid race conditions etc. ([#344][]) - Generate basic `.json` files with descriptions of the fields for `participants.tsv` and `_scans.tsv` files ([#376][]) - Use `filelock` while writing top level files. Use `HEUDICONV_FILELOCK_TIMEOUT` environment to change the default timeout value ([#348][]) - `_PDT2` was added as a suffix for multi-echo (really "multi-modal") sequences ([#345][]) - Calls to `dcm2niix` would include full output path to make it easier to discern in the logs what file it is working on ([#351][]) - With recent [datalad]() (>= 0.10), created DataLad dataset will use `--fake-dates` functionality of DataLad to not leak data conversion dates, which might be close to actual data acquisition/patient visit ([#352][]) - Support multi-echo EPI `_phase` data ([#373][] fixes [#368][]) - Log location of a bad .json file to ease troubleshooting ([#379][]) - Add basic pypi classifiers for the package ([#380][]) Fixed - Sorting `_scans.tsv` files lacking valid dates field should not cause a crash ([#337][]) - Multi-echo files detection based number of echos ([#339][]) - BIDS - Use `EchoTimes` from the associated multi-echo files if `EchoNumber` tag is missing ([#366][] fixes [#347][]) - Tolerate empty ContentTime and/or ContentDate in DICOMs ([#372][]) and place "n/a" if value is missing ([#390][]) - Do not crash and store original .json file is "JSON pretification" fails ([#342][]) - ReproIn heuristic - tollerate WIP prefix on Philips scanners ([#343][]) - allow for use of `(...)` instead of `{...}` since `{}` are not allowed ([#343][]) - Support pipolar fieldmaps by providing them with `_epi` not `_magnitude`. "Loose" BIDS `_key-value` pairs might come now after `_dir-` even if they came first before ([#358][] fixes [#357][]) - All heuristics saved under `.heudiconv/` under `heuristic.py` name, to avoid discrepancy during reconversion ([#354][] fixes [#353][]) - Do not crash (with TypeError) while trying to sort absent file list ([#360][]) - heudiconv requires nipype >= 1.0.0 ([#364][]) and blacklists `1.2.[12]` ([#375][]) * tag 'v0.6.0': (60 commits) Version boost to 0.6.0 DOC: populate detailed changelog for 0.6.0 and tune up formatting in previous one Fix miscellaneous typos in ReproIn heuristic file. BF: fix check for the sbatch (SLURM) not being available ENH: make test-compare-two-versions take any two worktrees, and just show diff if results already known Update heudiconv/convert.py apply @mgxd 's suggestions, adding a warning and a timeout environment variable need str typecast Use empty string not None Empty acq_time results in empty cell not 'n/a' DOC: Clarify tarball session handling remove repetitive import statement respond to review - add explicit py2 check - change file saving strategy - use logger instead of print fix remaning py2 errors MNT: Add Python support metadata to package fix some python2/3 incompatibilities add return data (accidently removed return) make content unicode test that load_json provides filename if invalid explicitly name invalid json ...
yarikoptic
added a commit
that referenced
this pull request
Dec 16, 2019
[0.6.0] - 2019-12-16 This is largely a bug fix. Metadata and order of `_key-value` fields in BIDS could change from the result of converting using previous versions, thus minor version boost. 14 people contributed to this release -- thanks [everyone](https://github.com/nipy/heudiconv/graphs/contributors)! Enhancement - Use [etelemetry](https://pypi.org/project/etelemetry) to inform about most recent available version of heudiconv. Please set `NO_ET` environment variable if you want to disable it ([#369][]) - BIDS: - `--bids` flag became an option. It can (optionally) accept `notop` value to avoid creation of top level files (`CHANGES`, `dataset_description.json`, etc) as a workaround during parallel execution to avoid race conditions etc. ([#344][]) - Generate basic `.json` files with descriptions of the fields for `participants.tsv` and `_scans.tsv` files ([#376][]) - Use `filelock` while writing top level files. Use `HEUDICONV_FILELOCK_TIMEOUT` environment to change the default timeout value ([#348][]) - `_PDT2` was added as a suffix for multi-echo (really "multi-modal") sequences ([#345][]) - Calls to `dcm2niix` would include full output path to make it easier to discern in the logs what file it is working on ([#351][]) - With recent [datalad]() (>= 0.10), created DataLad dataset will use `--fake-dates` functionality of DataLad to not leak data conversion dates, which might be close to actual data acquisition/patient visit ([#352][]) - Support multi-echo EPI `_phase` data ([#373][] fixes [#368][]) - Log location of a bad .json file to ease troubleshooting ([#379][]) - Add basic pypi classifiers for the package ([#380][]) Fixed - Sorting `_scans.tsv` files lacking valid dates field should not cause a crash ([#337][]) - Multi-echo files detection based number of echos ([#339][]) - BIDS - Use `EchoTimes` from the associated multi-echo files if `EchoNumber` tag is missing ([#366][] fixes [#347][]) - Tolerate empty ContentTime and/or ContentDate in DICOMs ([#372][]) and place "n/a" if value is missing ([#390][]) - Do not crash and store original .json file is "JSON pretification" fails ([#342][]) - ReproIn heuristic - tolerate WIP prefix on Philips scanners ([#343][]) - allow for use of `(...)` instead of `{...}` since `{}` are not allowed ([#343][]) - Support pipolar fieldmaps by providing them with `_epi` not `_magnitude`. "Loose" BIDS `_key-value` pairs might come now after `_dir-` even if they came first before ([#358][] fixes [#357][]) - All heuristics saved under `.heudiconv/` under `heuristic.py` name, to avoid discrepancy during reconversion ([#354][] fixes [#353][]) - Do not crash (with TypeError) while trying to sort absent file list ([#360][]) - heudiconv requires nipype >= 1.0.0 ([#364][]) and blacklists `1.2.[12]` ([#375][]) * tag 'v0.6.0': Boost perspective release date in changelog to today ENH(TST): Fix version to older pytest to ease backward compatibility testing RF: use tmpdir not tmp_path fixture FIX: minor typo in CHANGELOG.md
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
AFAIK a single echo time is not making it multi-echo, or does it?
I might go more through that code, so this PR will be evolving unless rapidly merged ;)