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.

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

bids_ME heuristic: add test for the dataset that raised #541, add support for MEGRE #547

Merged
merged 8 commits into from
Apr 5, 2022

Conversation

pvelasco
Copy link
Contributor

No description provided.

@yarikoptic
Copy link
Member

@pvelasco

  • note that travis isn't happy (didn't look inside)
  • although not that heavy dicoms, we keep growing that tests/data and it starts worry me. Should I just add MEGRE under http://datasets.datalad.org/?dir=/dicoms/velasco to stay near its MEEPI? ;)

In `test_ME_mag_phase_conversion`
@pvelasco
Copy link
Contributor Author

pvelasco commented Mar 1, 2022

Yes, please, @yarikoptic; add MEGRE to datalad. Then I will correct the tests.

@codecov
Copy link

codecov bot commented Mar 1, 2022

Codecov Report

Merging #547 (aee3d9b) into master (cb2fd91) will increase coverage by 0.03%.
The diff coverage is 86.20%.

@@            Coverage Diff             @@
##           master     #547      +/-   ##
==========================================
+ Coverage   81.21%   81.24%   +0.03%     
==========================================
  Files          41       41              
  Lines        3784     3813      +29     
==========================================
+ Hits         3073     3098      +25     
- Misses        711      715       +4     
Impacted Files Coverage Δ
heudiconv/tests/test_bids.py 98.69% <80.95%> (-1.31%) ⬇️
heudiconv/heuristics/bids_ME.py 94.44% <100.00%> (+3.53%) ⬆️
heudiconv/tests/test_main.py 100.00% <0.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 cb2fd91...aee3d9b. Read the comment docs.

@yarikoptic
Copy link
Member

Copy link
Member

@yarikoptic yarikoptic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test seems in need of fixing. To expedite, I will accept one of my changes, feel free to revert (well - just push --force your previous state) -- I just want to see quickly if may be it would resolve so I could accept

for s in seqinfo:
if '_ME_' in s.series_description:
info[bold].append(s.series_id)
if 'GRE_QSM' in s.series_description:
if s.image_type[2] == 'M':
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: this code block is not covered by tests... what data was supposed to have it covered?

try:
datadir = fetch_data(tmppath, "dicoms/velasco/{subID}")
except IncompleteResultsError as exc:
pytest.skip("Failed to fetch test data: %s" % str(exc))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note -- this line is "covered" which means that the test just skipped... I think I might see the reason

heudiconv/tests/test_bids.py Outdated Show resolved Hide resolved
heudiconv/tests/test_bids.py Outdated Show resolved Hide resolved
yarikoptic added a commit to datalad/datasets.datalad.org that referenced this pull request Mar 31, 2022
yarikoptic added a commit to datalad/datasets.datalad.org that referenced this pull request Mar 31, 2022
@yarikoptic
Copy link
Member

coolio, thank you @pvelasco !

@yarikoptic yarikoptic merged commit a1f63de into nipy:master Apr 5, 2022
@yarikoptic yarikoptic changed the title Add test for the dataset that raised #541 bids_ME heuristic: add test for the dataset that raised #541, add support for MEGRE Apr 20, 2022
@yarikoptic yarikoptic added the patch Increment the patch version when merged label Apr 20, 2022
@github-actions
Copy link

🚀 PR was released in v0.11.0 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch Increment the patch version when merged released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants