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

Calculate tumor volume #78

Merged
merged 1 commit into from Sep 15, 2017

Conversation

Projects
None yet
4 participants
@vessemer
Copy link
Contributor

vessemer commented Aug 27, 2017

Description

This method computes the volume of connected components placed at the specified centroids, via connected component analysis using scipy.ndimage.label. The voxel_shape parameter has added to the function in order to derive units of measure.

Reference to official issue

Calculate tumor volume from pixel masks #13

How Has This Been Tested?

The unit test for calculate_volume has been added to test for the volume calculation over the different mask instances. During the testing, another issue has acquired at test_segment, which was related to the meaningless segment_path. Hence the changes were also added to the segment.predict function.

CLA

  • I have signed the CLA; if other committers are in the commit history, they have signed the CLA as well
@WGierke

This comment has been minimized.

Copy link
Contributor

WGierke commented Aug 27, 2017

I'd suggest you to use flake8 here as well :)

➜  concept-to-clinic git:(13_calculate_tumor_volume) flake8 prediction
prediction/src/algorithms/segment/trained_model.py:53:66: W291 trailing whitespace
prediction/src/algorithms/segment/trained_model.py:54:81: W291 trailing whitespace
prediction/src/algorithms/segment/trained_model.py:62:77: W291 trailing whitespace
prediction/src/algorithms/segment/trained_model.py:63:65: W291 trailing whitespace
prediction/src/algorithms/segment/trained_model.py:65:12: W291 trailing whitespace
prediction/src/algorithms/segment/trained_model.py:67:1: W293 blank line contains whitespace
prediction/src/algorithms/segment/trained_model.py:76:1: W293 blank line contains whitespace
prediction/src/tests/test_calculate_volume.py:17:1: W293 blank line contains whitespace
prediction/src/tests/test_calculate_volume.py:24:1: W293 blank line contains whitespace
prediction/src/tests/test_calculate_volume.py:35:48: W291 trailing whitespace
prediction/src/tests/test_calculate_volume.py:38:1: W293 blank line contains whitespace
prediction/src/tests/test_calculate_volume.py:41:47: W291 trailing whitespace
prediction/src/tests/test_calculate_volume.py:43:1: W293 blank line contains whitespace
prediction/src/tests/test_calculate_volume.py:46:1: E303 too many blank lines (3)
prediction/src/tests/test_calculate_volume.py:52:1: W293 blank line contains whitespace
prediction/src/tests/test_calculate_volume.py:59:1: W293 blank line contains whitespace
prediction/src/tests/test_calculate_volume.py:64:1: W293 blank line contains whitespace
@vessemer

This comment has been minimized.

Copy link
Contributor

vessemer commented Aug 27, 2017

Thanks, @WGierke, I've followed your suggestion.

mask = generate_mask(shape=[50, 50, 29], centroids=centroids, volumes=[100, 20, 30])
assert mask.sum() == 150

path = os.path.join('.', 'tmp', 'mask.npy')

This comment has been minimized.

@lamby

lamby Aug 29, 2017

Contributor

Please use pylint's tmpdir support; it will — at the very least — clean up after itself on failing tests and avoid the need for ugly path manipulation!

This comment has been minimized.

@vessemer

vessemer Aug 29, 2017

Contributor

Thank you for pointing it out, I've fixed it.

def to_real_volume(volumes, dicom_path):
"""
Transforms the volumes, provided in voxel units, into the volumes in mm.
The metadata the DICOM.

This comment has been minimized.

@reubano

reubano Aug 31, 2017

Contributor

Looks like a typo here :)

@reubano

This comment has been minimized.

Copy link
Contributor

reubano commented Sep 1, 2017

Hi @vessemer thanks for the updates! I've just been trying to wrap my head around what you've done, so hopefully you can help me understand :).

Earlier you mentioned concern about not having access to the dicom metadata, and passed in a third voxel_shape parameter to the calculate_volume function to accommodate. I now see that third parameter is no longer present. Did you figure out a clever way to perform the calculation without it?

Also, what is the use case for to_real_volume? I see you have a test for it, but it isn't referenced by the predict function.

@reubano reubano referenced this pull request Sep 1, 2017

Closed

Calculate tumor volume from pixel masks #13

0 of 2 tasks complete
@vessemer

This comment has been minimized.

Copy link
Contributor

vessemer commented Sep 2, 2017

Hi @reubano, as you requested in #13 (comment),

While it would be possible for calculate_volume to take dicom_path as an argument, and then calculate centroids on its own, we prefer to uncouple those steps and have a canonical function dedicated to that task.

I've decoupled the functions so that now the calculate_volume returns the volume in voxels, and only to_real_volume takes into account the voxels' shape and transform the list of volumes in voxels into the list of volumes in mm.

@reubano

This comment has been minimized.

Copy link
Contributor

reubano commented Sep 4, 2017

Sorry if I wasn't clear. By "uncouple those steps", I was referring to not calculating centroids from a dicom_path passed to the calculate_volume function. So, the upshot of that was that calculate_volume would just accept a centroids parameter (which was calculated by the decoupled identify/trained_model/predict function).

@reubano

This comment has been minimized.

Copy link
Contributor

reubano commented Sep 5, 2017

So feel free to re-integrate to_real_volume unless you see an advantage for keeping it separate.

@reubano

This comment has been minimized.

Copy link
Contributor

reubano commented Sep 6, 2017

Hi @vessemer, just wanted to check and make sure everything is ok. Is there anything blocking you? I think if you address the to_real_volume function (either by incorporating it into the predict function, or indicating when and how it should be used) when we can move forward to getting this merged. Please let me know if anything is unclear or if you have any questions.

@vessemer vessemer force-pushed the vessemer:13_calculate_tumor_volume branch from 97bee9e to 9ad4cb4 Sep 6, 2017

@vessemer

This comment has been minimized.

Copy link
Contributor

vessemer commented Sep 6, 2017

@reubano I've addressed the to_real_volume by incorporating it into the calculate_volume function. I've also added the function load_meta into the load_dicom.py file, in order to uncouple ugly os.path.join(dicom_path, '*.dcm') from the calculate_volume. It's worth noting that there exists not only DICOM files, but also the .mhd/.raw which has been used e.g. in LUNA16. Thus it may be useful to support different formats as well.

@reubano
Copy link
Contributor

reubano left a comment

This is looking really good! Please see requested changes.


def test_calculate_volume_over_connected_components_with_dicom_path(get_mask_connected):
path, centroids = get_mask_connected
print(path, centroids)

This comment has been minimized.

@reubano

reubano Sep 7, 2017

Contributor

Was this print meant to be left in? Perhaps comment it out, or use an assert statement if the result of the printing is necessary?


def test_calculate_volume_over_connected_components(get_mask_connected):
path, centroids = get_mask_connected
print(path, centroids)

This comment has been minimized.

@reubano

reubano Sep 7, 2017

Contributor

Was this print meant to be left in? Perhaps comment it out, or use an assert statement if the result of the printing is necessary?

# Despite they are overlapped, the amount of volumes must have preserved
assert len(real_volumes) == len(voxels_volumes)
assert all([(mm <= 1.236 * vox) or (mm >= 1.234 * vox)
for vox, mm in zip(voxels_volumes, real_volumes)])

This comment has been minimized.

@reubano

reubano Sep 7, 2017

Contributor

That's a pretty large range :). Any chance this should be an and instead of an or and the directions reversed? If so, we could also simplify it like so 1.234 >= mm / vox >= 1.236.

This comment has been minimized.

@vessemer

vessemer Sep 7, 2017

Contributor

Done, just >= and <= are swaped.

# Every DICOM study preserve the same PixelSpacing along its sub files
voxel_shape = dicom_files[0].PixelSpacing
# Taking into account ijk -> xyz transformation
voxel_shape = np.prod([voxel_shape[1], voxel_shape[0], slice_thikness])

This comment has been minimized.

@reubano

reubano Sep 7, 2017

Contributor

slice_thikness--> slice_thickness :)

volumes = volumes[labels]

if dicom_path is None:
return volumes.tolist()

This comment has been minimized.

@reubano

reubano Sep 7, 2017

Contributor

So this will still let empty paths go through, and two return statements aren't really ideal. Would you be able to refactor into something like this?

if dicom_path:
    pass
else:
    pass

return volumes
@reubano

This comment has been minimized.

Copy link
Contributor

reubano commented Sep 7, 2017

It's worth noting that there exists not only DICOM files, but also the .mhd/.raw which has been used e.g. in LUNA16. Thus it may be useful to support different formats as well.

Nice observation! Would you like to submit an issue for this use case?

@vessemer vessemer force-pushed the vessemer:13_calculate_tumor_volume branch from 9ad4cb4 to a45ba7e Sep 7, 2017

@vessemer

This comment has been minimized.

Copy link
Contributor

vessemer commented Sep 7, 2017

Nice observation! Would you like to submit an issue for this use case?

I've created one, but I also guess that it'll be necessary to implement the class aimed at metadata standardisation, maybe along with all reading orchestration.

@vessemer vessemer force-pushed the vessemer:13_calculate_tumor_volume branch from a45ba7e to 66808db Sep 8, 2017

@reubano

This comment has been minimized.

Copy link
Contributor

reubano commented Sep 11, 2017

LGTM. This just needs to be squashed to one commit and it'll be ready to merge!

@reubano

This comment has been minimized.

Copy link
Contributor

reubano commented Sep 13, 2017

Hi @vessemer, please let me know if you need any assistance squashing the commits. You should be able to do something like git rebase -i master.

Calculate tumor volume
Add .npy to lfs

@vessemer vessemer force-pushed the vessemer:13_calculate_tumor_volume branch from 66808db to ebf3790 Sep 14, 2017

@reubano reubano merged commit 585a80e into drivendataorg:master Sep 15, 2017

2 checks passed

concept-to-clinic/cla @vessemer has signed the CLA.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@reubano

This comment has been minimized.

Copy link
Contributor

reubano commented Sep 15, 2017

Awesome @vessemer! :shipit:

@vessemer vessemer deleted the vessemer:13_calculate_tumor_volume branch Sep 29, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment