Merge util.metrics.py from CerebNet and FastSurferCNN into one file #590
Conversation
taha-abdullah
commented
Oct 8, 2024
taha-abdullah
commented
- Deleted CerebNet.utils.metrics.py and merged all functionality into FastSurferCNN.utils.metrics.py
- refactoring usage across files
- changed dice score implementation to numpy (tried MONAI and Ignite, didn't work out)
- added support for multiple images in test_images
- nested parameterization (subjects and test files) in test_images and test_stats
taha-abdullah
force-pushed
the
pytest
branch
2 times, most recently
from
fe5772b
to
80884ad
Compare
.github/workflows/quicktest.yaml
Outdated
There was a problem hiding this comment.
I do not understand why these changes are required. I think the previous version (before your changes here) was better. It has the whole pipeline grouped into one job.
I guess the "on" variable should be uncommented :)
There was a problem hiding this comment.
I commented out the on pull requests to dev and stable. These should only be added at the very end, when everything works (if at all). Until then it should be triggered manually for testing.
Even later I think - given the computational overhead - it makes sense to trigger this only for specific pull requests, that even touch any of the files involved in testing, and not just modification to doc strings etc.
There was a problem hiding this comment.
Commented out trigger on pull request and changed it back to last iteration
There was a problem hiding this comment.
@m-reuter One major disadvantage for this is that such workflows can only be triggered for branches that are part of the repository. I.e. we can no longer run them on all pull requests when we want to. At least I have not found a way to tigger them. The best solution is probably to always trigger them, but have a label in the PR that needs to be set to actually run this workflow.
FastSurferCNN/utils/metrics.py
Outdated
| device : str of torch.device, optional | ||
| Device specification in case of distributed computation usage. | ||
| In most of the cases, it can be defined as "cuda:local_rank" or "cuda" | ||
| if already set `torch.cuda.set_device(local_rank)`. By default, if a distributed process group is | ||
| initialized and available, device is set to `cuda`. |
There was a problem hiding this comment.
It seems to me that this metrics functionality of this class was previously implemented here and is now lost. Was this ever really used before. We should double-check. It might have been used for multi-gpu support or something.
It seems to me like FastSurfer still uses this (so at a minimum, it needs to be adapted there)
FastSurfer/FastSurferCNN/utils/meters.py
Line 75 in 462b00d
This device variable is being passed through the pipeline including utils/train.py but it only seems to be self.device which is initialized in init
FastSurfer/FastSurferCNN/train.py
Line 79 in 5bd139e
In the end you will need to test the train script with a minimal dataset to make sure it does not get broken. I will find such a dataset for you, but if can also be generated with generate_dataset.py
| ) | ||
| self.union[i, j] = self.union[i, j] + torch.sum(gt) + torch.sum(pred) | ||
|
|
||
| def update(self, output): |
There was a problem hiding this comment.
It seems to me like update always calls _update_union_intersection.
| for i, c1 in enumerate(self.class_ids): | ||
| gt = (labels_batch == c1).float() | ||
| for j, c2 in enumerate(self.class_ids): | ||
| pred = (batch_output == c2).float() | ||
| self.intersection[i, j] = self.intersection[i, j] + torch.sum( | ||
| torch.mul(gt, pred) | ||
| ) | ||
| self.union[i, j] = self.union[i, j] + torch.sum(gt) + torch.sum(pred) |
There was a problem hiding this comment.
Also, it seems like the original implementation had a condition here that would only compute the full confusion matrix if explicitly required. Most times, the diagonal of the confusion matrix is enough (see previous). And always computing the full confusion matrix is EXTREMELY computationally expensive. (Prohibitively so). So we should only compute the full confusion matrix if it is explicitly requested.
|
|
||
| def comput_dice_cnf(self): |
There was a problem hiding this comment.
This would be the confusion matrix :)
also added test configuration files for new filetypes rebased pytest-feture onto merge-metrics adding support for multiple images in test_images.py
…ferCNN.utils.metrics.py - Deleting CerebNet.utils.metrics.py - refactoring usage across files - Changing back to numpy dice score implementation - removing CerebNet.utils.metrics.py from docs
- quicktest.yaml: commenting out trigger on pull request - FastSurferCNN.utils.metrics.py: docstring changes
