-
Notifications
You must be signed in to change notification settings - Fork 4
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
Binary segmentation #28
base: dev
Are you sure you want to change the base?
Conversation
…=classification +module=mlp"
…mp/refactor/hydra
log hparams
add arguments for system instantiate add missing params to camus config
fix camus labels
…ital into tmp/hydra/yaml
…de module parameters
* Add softmax and mcdropout system * Add evaluation pipeline (Seg metrics, calibration overlap)
Remove resume/train/ckpt_path errors from runner Change default name to remove error.
Update networks for 1 class output
Co-authored-by: Nathan Painchaud <[email protected]>
…ture/binary-segmentation
Co-authored-by: Nathan Painchaud <[email protected]>
…ture/binary-segmentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't have to review all the modifications for now, but I commented on slight docstrings edits and on what required the most rework IMO (the image logging debug/dev code).
if len(self.labels) == 2: # For binary segmentation, make foreground class 1. | ||
proc_gts_tensor[proc_gts_tensor != 0] = 1 | ||
|
||
if len(self.labels) == 2: # For binary segmentation, make foreground class 1. | ||
gts[gts != 0] = 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both if
s should be merged.
@@ -35,32 +36,58 @@ def tversky_score( | |||
Available reduction methods: | |||
- ``'elementwise_mean'``: takes the mean (default) | |||
- ``'none'``: no reduction will be applied | |||
apply_activation: when True, softmax or sigmoid is applied to input. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd reword the doc slightly:
apply_activation: when True, softmax or sigmoid is applied to input. | |
apply_activation: If ``True``, applies softmax or sigmoid to the input before computing the score. Otherwise, assumes the input is already normalized. |
@@ -25,8 +26,8 @@ def tversky_score( | |||
https://lars76.github.io/2018/09/27/loss-functions-for-segmentation.html | |||
|
|||
Args: | |||
input: (N, C, H, W), Raw, unnormalized scores for each class. | |||
target: (N, H, W), Groundtruth labels, where each value is 0 <= targets[i] <= C-1. | |||
input: (N, C, H, W), Raw, unnormalized (or normalized apply_activation == False) scores for each class. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing if
+ formatting:
input: (N, C, H, W), Raw, unnormalized (or normalized apply_activation == False) scores for each class. | |
input: (N, C, H, W), Raw, unnormalized (or normalized if ``apply_activation == False``) scores for each class. |
) -> Tensor: | ||
"""Computes the loss definition of the dice coefficient. | ||
|
||
Args: | ||
input: (N, C, H, W), Raw, unnormalized scores for each class. | ||
input: (N, C, H, W), Raw, unnormalized (or normalized apply_activation == False) scores for each class. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as for tversky_score
.
@@ -83,12 +111,20 @@ def differentiable_dice_score( | |||
Available reduction methods: | |||
- ``'elementwise_mean'``: takes the mean (default) | |||
- ``'none'``: no reduction will be applied | |||
apply_activation: when True, softmax or sigmoid is applied to input. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as for tversky_score
.
@@ -23,19 +24,21 @@ def __init__( | |||
Available reduction methods: | |||
- ``'elementwise_mean'``: takes the mean (default) | |||
- ``'none'``: no reduction will be applied | |||
apply_activation: when True, softmax or sigmoid is applied to input. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as for the functional version.
|
||
def forward(self, input: Tensor, target: Tensor) -> Tensor: | ||
"""Actual metric calculation. | ||
|
||
Args: | ||
input: (N, C, H, W), Raw, unnormalized scores for each class. | ||
input: (N, C, H, W), Raw, unnormalized (or normalized apply_activation == False) if scores for each class. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as for the functional version.
"""Changes registry name to replace characters that COMET ML does not accept in the registry name. | ||
|
||
Note: Comet ML rules (Might need to be updated) | ||
- All special characters are replaced with dash (ex. camus_unet -> camus-unet, camus!unet -> camus-unet) | ||
- All non-letter and non-number characters at the end of the name are deleted. | ||
- All letters are lower-case. | ||
|
||
Args: | ||
registry_name: Name of the registered model to update. | ||
|
||
Returns: | ||
Updated registry name | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few edits:
"""Changes registry name to replace characters that COMET ML does not accept in the registry name. | |
Note: Comet ML rules (Might need to be updated) | |
- All special characters are replaced with dash (ex. camus_unet -> camus-unet, camus!unet -> camus-unet) | |
- All non-letter and non-number characters at the end of the name are deleted. | |
- All letters are lower-case. | |
Args: | |
registry_name: Name of the registered model to update. | |
Returns: | |
Updated registry name | |
""" | |
"""Changes registry names to replace characters that Comet ML does not accept. | |
Notes: Comet ML rules (Might need to be updated) | |
- All special characters are replaced with dash (ex. my_model -> my-model, my!model -> my-model) | |
- All non-letter and non-number characters at the end of the name are deleted. | |
- All letters are lower-case. | |
Args: | |
registry_name: Name of the registered model to fix. | |
Returns: | |
Fixed registry name. | |
""" |
registry_name = re.sub("[^a-zA-Z0-9]+", "-", registry_name) # Replace all special characters with `-` | ||
registry_name = re.sub(r"^\W+|\W+$", "", registry_name) # Remove all special characters at end of name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't use Python's regex often enough to remember how they work off the top of my head, so I'll trust you on that one 😉
if self.is_val_step and batch_idx == 0: | ||
self.log_images( | ||
title="Sample", | ||
num_images=5, | ||
axes_content={"Image": x.cpu().squeeze().numpy()}, | ||
info=[f"(Gt: {y[i].item()} - Pred: {y_hat.argmax(dim=1)[i].item()})" for i in range(5)], | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm in a bit of a rush to make a well thought-out suggestion, but to me this seems like debug/dev code, that we shouldn't ship as part of the model. I assume it would be possible to bundle both this bit of code and the log_images
added to VitalSystem
into a callback. And I would very much suggest to do it that way.
Add support for binary segmentation.
Changes were made to the Dice loss, segmentation system and Camus dataset.
Other minor changes in this PR (sorry for the random changes)