Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ENH: Replace rec entity with part entity for complex-valued data #477

Merged
merged 3 commits into from
Dec 8, 2020
Merged

ENH: Replace rec entity with part entity for complex-valued data #477

merged 3 commits into from
Dec 8, 2020

Conversation

tsalo
Copy link
Member

@tsalo tsalo commented Dec 7, 2020

Closes #476.

Changes proposed:

  • Replace rec-<magnitude|phase> with part-<mag|phase> now that BIDS supports the part entity.
  • Update associated test.

@codecov
Copy link

codecov bot commented Dec 8, 2020 •
edited
Loading

Codecov Report

Merging #477 (b1a3e53) into master (d855f64) will not change coverage.
The diff coverage is 87.50%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #477   +/-   ##
=======================================
  Coverage   76.13%   76.13%           
=======================================
  Files          37       37           
  Lines        2962     2962           
=======================================
  Hits         2255     2255           
  Misses        707      707
Impacted Files Coverage Δ
heudiconv/convert.py 84.90% <80.00%> (ø)
heudiconv/tests/test_convert.py 100.00% <100.00%> (ø)

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 d855f64...b1a3e53. Read the comment docs.

@yarikoptic
Copy link
Member

I am all of the change and great to have BIDS supporting it (thank you @tsalo for making it happen).
My questions/concerns:

@tsalo
Copy link
Member Author

tsalo commented Dec 8, 2020 •
edited
Loading

is BIDS already released with this change?

part (and inv and flip) haven't been released yet. I believe they will be fairly soon though (i.e., as soon as BEP005 and/or BEP001 are fully merged in). EDIT: And by fairly soon I'm thinking early January or before.

are you aware of any processing pipeline/app which could have relied on _rec- and now would get broken if we start using _part-?

I am not aware of any. Possibly phaseprep, but that hasn't had any releases yet, and I can open an issue to be safe. I'm pretty sure that most pipelines that should take part into account don't do so even with rec already in use. For example, fMRIPrep doesn't deselect single-band reference images with rec-phase at the moment (which could cause problems if you have complex-valued single-band reference images and fMRIPrep tries to use them for reference image creation), so I will need to follow up there and possibly elsewhere.

Also, support for part-phase + _bold is new (and indeed rec-phase + _bold wasn't support here) because the _phase suffix was not deprecated until this upcoming release. As a result, a number of tools will need to actively deselect phase data with the _bold suffix anyway, whether using rec or part.

@yarikoptic
Copy link
Member

so it seems that it should be safe to proceed. We would just not forget to make it a "major" (well "0.minor") release with a corresponding CHANGELOG entry. Thank you!

@yarikoptic yarikoptic merged commit b6eedc7 into nipy:master Dec 8, 2020
@tsalo tsalo deleted the rec-to-part branch December 8, 2020 16:43
@yarikoptic yarikoptic added this to the 0.9.0 milestone Dec 23, 2020
yarikoptic added a commit that referenced this pull request Dec 23, 2020
@yarikoptic
Merge tag 'v0.9.0' into debian
0c78a20 
Various improvements and compatibility/support (dcm2niix, datalad,
duecredit) changes.  Major change is placement of output files to the
target output directory during conversion.

- #454 zenodo referencing in README.rst and support for ducredit for
  heudiconv and reproin heuristic
- #445 more tutorial references in README.md

- [#485][] placed files during conversion right away into the target
  directory (with a `_heudiconv???` suffix, renamed into ultimate target
  name later on), which avoids hitting file size limits of /tmp ([#481][]) and
  helped to avoid a regression in dcm2nixx 1.0.20201102
- #477 replaced `rec-<magnitude|phase>` with `part-<mag|phase>` now
  that BIDS supports the part entity
- #473 made default for CogAtlasID to be a TODO URL
- #459 made AcquisitionTime used for acq_time scans file field
- #451 retained sub-second resolution in scans files
- #442 refactored code so there is now heudiconv.main.workflow for
  more convenient use as a Python module

- minimal version of nipype set to 1.2.3 to guarantee correct handling
  of DWI files ([#480][])
- `heudiconvDCM*` temporary directories are removed now ([#462][])
- compatibility with DataLad 0.13 ([#464][])

- #443 pathlib as a dependency (we are Python3 only now)

* tag 'v0.9.0':
  Add a helper rule to upload to pypi
  update changelog reference as part of prep release
  [DATALAD RUNCMD] prepare the release
  CHANGELOG entry for 0.9.0
yarikoptic added a commit that referenced this pull request Dec 23, 2020
@yarikoptic
Merge tag 'v0.9.0' into debian
c80762a 
Various improvements and compatibility/support (dcm2niix, datalad,
duecredit) changes.  Major change is placement of output files to the
target output directory during conversion.

- #454 zenodo referencing in README.rst and support for ducredit for
  heudiconv and reproin heuristic
- #445 more tutorial references in README.md

- [#485][] placed files during conversion right away into the target
  directory (with a `_heudiconv???` suffix, renamed into ultimate target
  name later on), which avoids hitting file size limits of /tmp ([#481][]) and
  helped to avoid a regression in dcm2nixx 1.0.20201102
- #477 replaced `rec-<magnitude|phase>` with `part-<mag|phase>` now
  that BIDS supports the part entity
- #473 made default for CogAtlasID to be a TODO URL
- #459 made AcquisitionTime used for acq_time scans file field
- #451 retained sub-second resolution in scans files
- #442 refactored code so there is now heudiconv.main.workflow for
  more convenient use as a Python module

- minimal version of nipype set to 1.2.3 to guarantee correct handling
  of DWI files ([#480][])
- `heudiconvDCM*` temporary directories are removed now ([#462][])
- compatibility with DataLad 0.13 ([#464][])

- #443 pathlib as a dependency (we are Python3 only now)

* tag 'v0.9.0':
  Do no bother ensuring that version changed - should be no changes
@yarikoptic yarikoptic mentioned this pull request Dec 19, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.9.0
Development

Successfully merging this pull request may close these issues.

Replace rec entity with part entity for complex-valued data
2 participants
@tsalo @yarikoptic