Implement TIGER TIL task #885
Conversation
jklubienski
force-pushed
the
feature/tiger-til
branch
from
77b2937 to
b689cb1
Compare
nkaenzig
left a comment
nkaenzig
left a comment
There was a problem hiding this comment.
Looks great 👍. I left a couple of comments.
configs/vision/pathology/offline/regression/tiger_til_score.yaml
Outdated
Show resolved
Hide resolved
configs/vision/pathology/offline/regression/tiger_til_score.yaml
Outdated
Show resolved
Hide resolved
jklubienski
force-pushed
the
feature/tiger-til
branch
from
fc336a3 to
ef84783
Compare
nkaenzig
left a comment
nkaenzig
left a comment
There was a problem hiding this comment.
Great work 🚀 Left a few last small comments. Last thing missing would be the unit tests as discussed offline.
| save_last: ${oc.env:SAVE_LAST, false} | ||
| save_top_k: 1 | ||
| monitor: &MONITOR_METRIC ${oc.env:MONITOR_METRIC, val/MAE} | ||
| mode: &MONITOR_METRIC_MODE ${oc.env:MONITOR_METRIC_MODE, max} |
There was a problem hiding this comment.
monitors val/MAE with mode: max, so checkpoints and early stopping will keep the worst models rather than the best.
| head: | ||
| class_path: eva.vision.models.networks.ABMIL | ||
| init_args: | ||
| input_size: ${oc.env:IN_FEATURES, 384} |
There was a problem hiding this comment.
Do we need to set projected_input_size here? (ABMIL doesn't have a default value for that, so instantiation should fail without setting it)
| max_samples: *N_PATCHES | ||
| width: 224 | ||
| height: 224 | ||
| target_mpp: 0.5 |
There was a problem hiding this comment.
This init arg was removed from the tiger dataset class.
There was a problem hiding this comment.
Plz add this config to tests/eva/vision/test_vision_cli.py (at least to test_configuration_initialization, ideally also to test_predict_fit_from_configuration), so we can test for instantiation errors
| """Embeddings dataset class for regression tasks.""" | ||
|
|
||
| @override | ||
| def load_target(self, index: int) -> torch.Tensor: |
There was a problem hiding this comment.
Inconsistent Return Types - embeddings.py:13 vs multi_embeddings.py:96
- EmbeddingsRegressionDataset returns torch.Tensor
- MultiEmbeddingsDataset returns npt.NDArray[Any]
- This inconsistency could cause type confusion
| import torch | ||
| from typing_extensions import override | ||
|
|
||
| from eva.core.data.datasets import embeddings as embeddings_base |
There was a problem hiding this comment.
nit: from eva.core.data.datasets import embeddings as base (for conciseness)
| _train_split_ratio: float = 0.7 | ||
| _val_split_ratio: float = 0.15 | ||
|
|
||
| # target microns per pixel (mpp) for patches. |
There was a problem hiding this comment.
nit: usually we comment class attributes like this:
_target_mpp: float = 0.5
```Target microns per pixel (mpp) for patches.```2 participants
Adds TIGER TIL regression task to eva using the WSITILS dataset from the TIGER grand challenge (
tiger_tils.py), along with a suggested config to run it (tiger_tils.yaml)Also implements regression metrics and adds extensible support for future regression tasks in the file system.
I realise here that some of the code has been reused from the classification side (e.g. for the embeddings). If the prevailing opinion is that it's best practice to have less duplicate code but make everything slightly less readable file structure-wise this is very easy for me to change.
Testing to come precipitantly.