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

Add Mask transform #527

Merged
merged 9 commits into from
Jun 1, 2021
Merged

Add Mask transform #527

merged 9 commits into from
Jun 1, 2021

Conversation

Svdvoort
Copy link
Contributor

Fixes #521.

Description

Preprocessing option to set values outside of a LabelMap to a constant value (0 by default).
Also allows to select the specific Labels from the LabelMap that should be considered 'foreground' in case there are multiple Labels.

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

Update masking documentation

Add masking function to interface

Added masking example, corrected typing

Add tests

Make the grid sampler compatible with the queue (#520)

* Make GridSampler compatible with Queue

Resolves #513.

* Update docs

Bump version: 0.18.36 → 0.18.37
@codecov
Copy link

codecov bot commented May 10, 2021

Codecov Report

Merging #527 (a08788a) into master (af86828) will increase coverage by 0.15%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #527      +/-   ##
==========================================
+ Coverage   96.68%   96.83%   +0.15%     
==========================================
  Files         120      122       +2     
  Lines        6279     6358      +79     
==========================================
+ Hits         6071     6157      +86     
+ Misses        208      201       -7     
Impacted Files Coverage Δ
tests/transforms/preprocessing/test_mask.py 100.00% <100.00%> (ø)
torchio/data/subject.py 96.66% <100.00%> (+0.04%) ⬆️
torchio/transforms/__init__.py 100.00% <100.00%> (ø)
torchio/transforms/intensity_transform.py 100.00% <100.00%> (ø)
torchio/transforms/preprocessing/__init__.py 100.00% <100.00%> (ø)
torchio/transforms/preprocessing/intensity/mask.py 100.00% <100.00%> (ø)
torchio/transforms/transform.py 96.93% <100.00%> (+0.05%) ⬆️
torchio/utils.py 93.29% <0.00%> (+4.26%) ⬆️

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 af86828...a08788a. Read the comment docs.

@fepegar
Copy link
Owner

fepegar commented May 11, 2021

Hi, @Svdvoort. This looks good. I have added some changes. I suspect the labels features might not be working. I tried this code (also tried it before my changes) unsuccessfully:

import torchio as tio

colin = tio.datasets.Colin27(2008)
mask = tio.Mask(masking_method='cls', labels=[1, 2, 3, 12])
masked = mask(colin)
masked.plot()

I'll try to add a failing test.

@fepegar
Copy link
Owner

fepegar commented May 11, 2021

Is there maybe a copy-paste issue inside test_mask_specified_label that makes this test not work as expected?

@fepegar fepegar changed the title 521 mask intensity value Add Mask transform May 11, 2021
@Svdvoort
Copy link
Contributor Author

After some testing I think it's due to the way get_mask_from_masking_method returns the mask. I expected it to be the actual values of the LabelMap, so in the case of the example [0, 1, 2, 3, 4], but on line 436 it is already turned into boolean. So in this case it returns [False, True, True, True, True]. The specified labels [1, 2] are essentially [True, True], so it only masks the first value of image_tensor.

Is it intended that the returned mask is only boolean? If so what do you propose as an alternate solution to get the LabelMap?

@fepegar
Copy link
Owner

fepegar commented May 12, 2021

After some testing I think it's due to the way get_mask_from_masking_method returns the mask. I expected it to be the actual values of the LabelMap, so in the case of the example [0, 1, 2, 3, 4], but on line 436 it is already turned into boolean. So in this case it returns [False, True, True, True, True]. The specified labels [1, 2] are essentially [True, True], so it only masks the first value of image_tensor.

You are right, thanks for investigating.

Is it intended that the returned mask is only boolean?

I think it makes sense that get_mask_from_masking_method returns a binary image, yes. What about adding a labels kwarg to that method and compute the mask using the labels if they are not None?

@Svdvoort
Copy link
Contributor Author

I think it makes sense that get_mask_from_masking_method returns a binary image, yes. What about adding a labels kwarg to that method and compute the mask using the labels if they are not None?

With this labels argument, it would still return a binary image? So give it labels=[1, 3, 4] would return a binary mask for only those labels? It would solve this issue, but I can imagine that for other use cases it might also be handy to have the actual mask (with the labels, not as a binary image) returned. We could also add a return_labels argument which is False by default, but if it is True it would return the mask with the labels instead of a binary image?

However, if the underlying assumption is always that the function should return a binary image then I agree with the first approach.

@fepegar
Copy link
Owner

fepegar commented May 20, 2021

I think it's safe to assume that a binary image is returned. We can modify the behavior later on, if new use cases are needed.

@Svdvoort
Copy link
Contributor Author

Svdvoort commented Jun 1, 2021

I've added an option to return a binary mask from only the requested labels, and updated this in the mask function, it now passes the tests.

@fepegar
Copy link
Owner

fepegar commented Jun 1, 2021

Thanks! I'll fix the conflict and merge ASAP.

@fepegar fepegar merged commit a51248e into fepegar:master Jun 1, 2021
@fepegar
Copy link
Owner

fepegar commented Jun 1, 2021

Thanks for your contribution, @Svdvoort!

@all-contributors please add @Svdvoort for code

@allcontributors
Copy link
Contributor

@fepegar

I've put up a pull request to add @Svdvoort! 🎉

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.

Mask values outside of a LabelMap
2 participants