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

Fix affine getter for multipath images #763

Merged
merged 2 commits into from
Nov 29, 2021
Merged

Conversation

fepegar
Copy link
Owner

@fepegar fepegar commented Nov 29, 2021

Fixes #762.

Description
An error was being raised when trying to get the affine of multipath images, as the list of paths was being passed to the SimpleITK reader. Now, the images are loaded before returning the affine if the image is multipath.

Checklist

  • I have read the CONTRIBUTING docs and have a developer setup (especially important are pre-commitand pytest)
  • Non-breaking change (would not break existing functionality)
  • Breaking change (would cause existing functionality to change)
  • Tests added or modified to cover the changes
  • Integration tests passed locally by running pytest
  • In-line docstrings updated
  • Documentation updated, tested running make html inside the docs/ folder
  • This pull request is ready to be reviewed
  • If the PR is ready and there are multiple commits, I have squashed them and force-pushed

@dmus

@codecov
Copy link

codecov bot commented Nov 29, 2021

Codecov Report

Merging #763 (11f3695) into main (f0abd6b) will decrease coverage by 0.00%.
The diff coverage is 95.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #763      +/-   ##
==========================================
- Coverage   93.88%   93.88%   -0.01%     
==========================================
  Files          72       72              
  Lines        4499     4514      +15     
==========================================
+ Hits         4224     4238      +14     
- Misses        275      276       +1     
Impacted Files Coverage Δ
torchio/data/image.py 87.50% <95.00%> (+0.24%) ⬆️

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 f0abd6b...11f3695. Read the comment docs.

@dmus
Copy link
Contributor

dmus commented Nov 29, 2021

Quick fix :) Now you load the image when it is a multipath image. Another option would be to only call read_affine with the first element of the list when it is a multipath? Or is that option not preferable?

@fepegar fepegar merged commit 062fc96 into main Nov 29, 2021
@fepegar fepegar deleted the 762-fix-multipath-affine branch November 29, 2021 20:30
@fepegar
Copy link
Owner Author

fepegar commented Nov 29, 2021

Yes, that's another option. But I thought this would be safer, as it checks that all affines in the input images are the same (I think). I thought that this compromise of speed vs robustness was worth it as passing a sequence of paths is probably a not very common use case. Does that make sense?

@fepegar fepegar restored the 762-fix-multipath-affine branch July 4, 2022 22:01
@fepegar fepegar deleted the 762-fix-multipath-affine branch July 4, 2022 22:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error when using a list of paths in Image class
2 participants