Experiment/hybrid mil eval - #32
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
📝 WalkthroughWalkthroughThe PR adds structured MIL test outputs, specialized curve, histogram, metric, and prediction callbacks, and Hydra configurations for hybrid PGP and Virchow2 training, testing, prediction, and threshold estimation. ChangesMIL evaluation callbacks
Hybrid model experiment configuration
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Trainer
participant ProstateCancerAttentionMIL
participant EvaluationCallbacks
participant MLflow
Trainer->>ProstateCancerAttentionMIL: run test or prediction batch
ProstateCancerAttentionMIL-->>Trainer: return MILModelOutput
Trainer->>EvaluationCallbacks: invoke batch-end hooks
EvaluationCallbacks->>EvaluationCallbacks: collect masked outputs and metadata
Trainer->>EvaluationCallbacks: invoke epoch-end hooks
EvaluationCallbacks->>MLflow: log curves, histograms, metrics, or predictions
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
prostate_cancer/callbacks/curves_callback_base.py (1)
111-127: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSkip plotting when buffers are empty.
on_test_epoch_end()andon_predict_epoch_end()run on the base classes even when no batch callbacks added data.
prostate_cancer/callbacks/curves_callback_base.py#L111-L127: return beforetorch.cat(...)ifself.predsorself.targetsis empty.prostate_cancer/callbacks/tile_histograms_callback_base.py#L19-L60: return beforenp.concatenate(...)ifself.all_predsorself.all_labelsis empty.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@prostate_cancer/callbacks/curves_callback_base.py` around lines 111 - 127, Guard _plot_and_clear in prostate_cancer/callbacks/curves_callback_base.py (lines 111-127) by returning before torch.cat when either self.preds or self.targets is empty. Apply the same early-return guard in the plotting method in prostate_cancer/callbacks/tile_histograms_callback_base.py (lines 19-60) before np.concatenate when either self.all_preds or self.all_labels is empty.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@configs/model/pgp_hybrid_mil_model.yaml`:
- Around line 3-5: Bind the required lr, tl_threshold, and sl_threshold
parameters for ProstateCancerAttentionMIL using the shared experiment/default
configuration or explicit valid overrides. Apply the same change to
configs/model/pgp_hybrid_mil_model.yaml lines 3-5 and
configs/model/virchow2_hybrid_mil_model.yaml lines 3-5, removing the mandatory
??? placeholders so Hydra can instantiate both models.
In `@prostate_cancer/callbacks/curves_callback_base.py`:
- Line 13: Update CurvesCallbackBase so the ROC and precision-recall artifact
names generated in its plotting methods include the callback level instead of
fixed tile-level names. Also update TileHistogramsCallbackBase to include its
slide/tile level in the histograms artifact name, or move that naming into
subclasses, ensuring artifacts do not collide in the shared MLflow plots path.
Apply the changes in prostate_cancer/callbacks/curves_callback_base.py (anchor,
line 13) and prostate_cancer/callbacks/tile_histograms_callback_base.py
(sibling, lines 12-19).
In `@prostate_cancer/callbacks/nested_metrics_callback_mil.py`:
- Around line 26-31: Update the keys construction in the nested metrics callback
to call nonzero() once on mask_bool, then use the resulting selected row indices
to assemble slide names from metadata. Remove the nested per-tile iteration and
scalar if condition so GPU synchronization occurs only during the single index
materialization.
---
Outside diff comments:
In `@prostate_cancer/callbacks/curves_callback_base.py`:
- Around line 111-127: Guard _plot_and_clear in
prostate_cancer/callbacks/curves_callback_base.py (lines 111-127) by returning
before torch.cat when either self.preds or self.targets is empty. Apply the same
early-return guard in the plotting method in
prostate_cancer/callbacks/tile_histograms_callback_base.py (lines 19-60) before
np.concatenate when either self.all_preds or self.all_labels is empty.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 03830ea8-6940-46ae-ba68-6685b68d0e18
📒 Files selected for processing (49)
configs/callbacks/combined_test_suite_mil.yamlconfigs/callbacks/curves.yamlconfigs/callbacks/curves_mil.yamlconfigs/callbacks/curves_sl.yamlconfigs/callbacks/nested_metrics_mil.yamlconfigs/callbacks/slide_histograms_mil.yamlconfigs/callbacks/slide_threshold_mil.yamlconfigs/callbacks/tile_histograms.yamlconfigs/callbacks/tile_histograms_mil.yamlconfigs/callbacks/tile_threshold_mil.yamlconfigs/experiment/predict/bag_of_embeddings/pgp/base.yamlconfigs/experiment/predict/bag_of_embeddings/pgp/mil_predictions/base.yamlconfigs/experiment/predict/bag_of_embeddings/pgp/mil_predictions/mmci_sl_group_1.yamlconfigs/experiment/predict/bag_of_embeddings/pgp/mil_predictions/mmci_tl_val.yamlconfigs/experiment/predict/bag_of_embeddings/pgp/slide_threshold.yamlconfigs/experiment/predict/bag_of_embeddings/virchow2/base.yamlconfigs/experiment/predict/bag_of_embeddings/virchow2/mil_predictions/mmci_sl_group_1.yamlconfigs/experiment/predict/bag_of_embeddings/virchow2/slide_threshold.yamlconfigs/experiment/test/bag_of_embeddings/base.yamlconfigs/experiment/test/bag_of_embeddings/pgp/base.yamlconfigs/experiment/test/bag_of_embeddings/pgp/test_suite/base.yamlconfigs/experiment/test/bag_of_embeddings/pgp/test_suite/mmci_tl_test.yamlconfigs/experiment/test/bag_of_embeddings/pgp/tile_threshold.yamlconfigs/experiment/test/bag_of_embeddings/virchow2/base.yamlconfigs/experiment/test/bag_of_embeddings/virchow2/test_suite/base.yamlconfigs/experiment/test/bag_of_embeddings/virchow2/test_suite/mmci_tl_test.yamlconfigs/experiment/test/bag_of_embeddings/virchow2/tile_threshold.yamlconfigs/experiment/train/bag_of_embeddings/pgp.yamlconfigs/experiment/train/bag_of_embeddings/virchow2.yamlconfigs/model/checkpoints/mmci_tl/pgp_hybrid_mil.yamlconfigs/model/checkpoints/mmci_tl/virchow2_hybrid_mil.yamlconfigs/model/pgp_hybrid_mil_model.yamlconfigs/model/virchow2_hybrid_mil_model.yamlpreprocessing/embeddings/tile_embeddings.pypreprocessing/masks/quality_control_v2.pyprostate_cancer/attention_mil_model.pyprostate_cancer/callbacks/__init__.pyprostate_cancer/callbacks/curves_callback_base.pyprostate_cancer/callbacks/curves_callback_mil.pyprostate_cancer/callbacks/curves_callback_sl.pyprostate_cancer/callbacks/curves_callback_tl.pyprostate_cancer/callbacks/mil_prediction_callback.pyprostate_cancer/callbacks/nested_metrics_callback.pyprostate_cancer/callbacks/nested_metrics_callback_base.pyprostate_cancer/callbacks/nested_metrics_callback_mil.pyprostate_cancer/callbacks/slide_histograms_callback_mil.pyprostate_cancer/callbacks/tile_histograms_callback_base.pyprostate_cancer/callbacks/tile_histograms_callback_mil.pyprostate_cancer/callbacks/tile_histograms_callback_tl.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
prostate_cancer/callbacks/curves_callback_base.py (1)
111-127: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSkip plotting when buffers are empty.
on_test_epoch_end()andon_predict_epoch_end()run on the base classes even when no batch callbacks added data.
prostate_cancer/callbacks/curves_callback_base.py#L111-L127: return beforetorch.cat(...)ifself.predsorself.targetsis empty.prostate_cancer/callbacks/tile_histograms_callback_base.py#L19-L60: return beforenp.concatenate(...)ifself.all_predsorself.all_labelsis empty.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@prostate_cancer/callbacks/curves_callback_base.py` around lines 111 - 127, Guard _plot_and_clear in prostate_cancer/callbacks/curves_callback_base.py (lines 111-127) by returning before torch.cat when either self.preds or self.targets is empty. Apply the same early-return guard in the plotting method in prostate_cancer/callbacks/tile_histograms_callback_base.py (lines 19-60) before np.concatenate when either self.all_preds or self.all_labels is empty.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@configs/model/pgp_hybrid_mil_model.yaml`:
- Around line 3-5: Bind the required lr, tl_threshold, and sl_threshold
parameters for ProstateCancerAttentionMIL using the shared experiment/default
configuration or explicit valid overrides. Apply the same change to
configs/model/pgp_hybrid_mil_model.yaml lines 3-5 and
configs/model/virchow2_hybrid_mil_model.yaml lines 3-5, removing the mandatory
??? placeholders so Hydra can instantiate both models.
In `@prostate_cancer/callbacks/curves_callback_base.py`:
- Line 13: Update CurvesCallbackBase so the ROC and precision-recall artifact
names generated in its plotting methods include the callback level instead of
fixed tile-level names. Also update TileHistogramsCallbackBase to include its
slide/tile level in the histograms artifact name, or move that naming into
subclasses, ensuring artifacts do not collide in the shared MLflow plots path.
Apply the changes in prostate_cancer/callbacks/curves_callback_base.py (anchor,
line 13) and prostate_cancer/callbacks/tile_histograms_callback_base.py
(sibling, lines 12-19).
In `@prostate_cancer/callbacks/nested_metrics_callback_mil.py`:
- Around line 26-31: Update the keys construction in the nested metrics callback
to call nonzero() once on mask_bool, then use the resulting selected row indices
to assemble slide names from metadata. Remove the nested per-tile iteration and
scalar if condition so GPU synchronization occurs only during the single index
materialization.
---
Outside diff comments:
In `@prostate_cancer/callbacks/curves_callback_base.py`:
- Around line 111-127: Guard _plot_and_clear in
prostate_cancer/callbacks/curves_callback_base.py (lines 111-127) by returning
before torch.cat when either self.preds or self.targets is empty. Apply the same
early-return guard in the plotting method in
prostate_cancer/callbacks/tile_histograms_callback_base.py (lines 19-60) before
np.concatenate when either self.all_preds or self.all_labels is empty.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 03830ea8-6940-46ae-ba68-6685b68d0e18
📒 Files selected for processing (49)
configs/callbacks/combined_test_suite_mil.yamlconfigs/callbacks/curves.yamlconfigs/callbacks/curves_mil.yamlconfigs/callbacks/curves_sl.yamlconfigs/callbacks/nested_metrics_mil.yamlconfigs/callbacks/slide_histograms_mil.yamlconfigs/callbacks/slide_threshold_mil.yamlconfigs/callbacks/tile_histograms.yamlconfigs/callbacks/tile_histograms_mil.yamlconfigs/callbacks/tile_threshold_mil.yamlconfigs/experiment/predict/bag_of_embeddings/pgp/base.yamlconfigs/experiment/predict/bag_of_embeddings/pgp/mil_predictions/base.yamlconfigs/experiment/predict/bag_of_embeddings/pgp/mil_predictions/mmci_sl_group_1.yamlconfigs/experiment/predict/bag_of_embeddings/pgp/mil_predictions/mmci_tl_val.yamlconfigs/experiment/predict/bag_of_embeddings/pgp/slide_threshold.yamlconfigs/experiment/predict/bag_of_embeddings/virchow2/base.yamlconfigs/experiment/predict/bag_of_embeddings/virchow2/mil_predictions/mmci_sl_group_1.yamlconfigs/experiment/predict/bag_of_embeddings/virchow2/slide_threshold.yamlconfigs/experiment/test/bag_of_embeddings/base.yamlconfigs/experiment/test/bag_of_embeddings/pgp/base.yamlconfigs/experiment/test/bag_of_embeddings/pgp/test_suite/base.yamlconfigs/experiment/test/bag_of_embeddings/pgp/test_suite/mmci_tl_test.yamlconfigs/experiment/test/bag_of_embeddings/pgp/tile_threshold.yamlconfigs/experiment/test/bag_of_embeddings/virchow2/base.yamlconfigs/experiment/test/bag_of_embeddings/virchow2/test_suite/base.yamlconfigs/experiment/test/bag_of_embeddings/virchow2/test_suite/mmci_tl_test.yamlconfigs/experiment/test/bag_of_embeddings/virchow2/tile_threshold.yamlconfigs/experiment/train/bag_of_embeddings/pgp.yamlconfigs/experiment/train/bag_of_embeddings/virchow2.yamlconfigs/model/checkpoints/mmci_tl/pgp_hybrid_mil.yamlconfigs/model/checkpoints/mmci_tl/virchow2_hybrid_mil.yamlconfigs/model/pgp_hybrid_mil_model.yamlconfigs/model/virchow2_hybrid_mil_model.yamlpreprocessing/embeddings/tile_embeddings.pypreprocessing/masks/quality_control_v2.pyprostate_cancer/attention_mil_model.pyprostate_cancer/callbacks/__init__.pyprostate_cancer/callbacks/curves_callback_base.pyprostate_cancer/callbacks/curves_callback_mil.pyprostate_cancer/callbacks/curves_callback_sl.pyprostate_cancer/callbacks/curves_callback_tl.pyprostate_cancer/callbacks/mil_prediction_callback.pyprostate_cancer/callbacks/nested_metrics_callback.pyprostate_cancer/callbacks/nested_metrics_callback_base.pyprostate_cancer/callbacks/nested_metrics_callback_mil.pyprostate_cancer/callbacks/slide_histograms_callback_mil.pyprostate_cancer/callbacks/tile_histograms_callback_base.pyprostate_cancer/callbacks/tile_histograms_callback_mil.pyprostate_cancer/callbacks/tile_histograms_callback_tl.py
🛑 Comments failed to post (1)
configs/model/pgp_hybrid_mil_model.yaml (1)
3-5: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -eu rg -n -C2 '^(lr|tl_threshold|sl_threshold): \?\?\?$|model\.(lr|tl_threshold|sl_threshold)' \ configs/model/pgp_hybrid_mil_model.yaml \ configs/model/virchow2_hybrid_mil_model.yaml \ configs/experiment/train/bag_of_embeddings/pgp.yaml \ configs/experiment/train/bag_of_embeddings/virchow2.yamlRepository: RationAI/prostate-cancer
Length of output: 885
🏁 Script executed:
#!/bin/bash set -eu echo "== candidate config files ==" for f in configs/config.yaml configs/experiment/train/bag_of_embeddings/pgp.yaml configs/experiment/train/bag_of_embeddings/virchow2.yaml configs/model/*.yaml; do [ -f "$f" ] || continue echo "--- $f" sed -n '1,120p' "$f" done echo echo "== model class constructor/defaults ==" rg -n "class ProstateCancerAttentionMIL|def __init__|lr|tl_threshold|sl_threshold|foundation" prostate_cancer/attention_mil_model.py sed -n '1,220p' prostate_cancer/attention_mil_model.py echo echo "== usages of trainer/model config keys ==" rg -n "model=|--model=|model:" configs --glob '*.yaml'Repository: RationAI/prostate-cancer
Length of output: 11757
Bind the required hybrid-model parameters.
ProstateCancerAttentionMILrequireslr,tl_threshold, andsl_threshold; these selected configs currently leave them as mandatory???, so Hydra instantiation fails before training starts. Wire these to the shared experiment/default learning-rate and threshold configuration or provide fixed value overrides.
configs/model/pgp_hybrid_mil_model.yaml#L3-L5configs/model/virchow2_hybrid_mil_model.yaml#L3-L5📍 Affects 2 files
configs/model/pgp_hybrid_mil_model.yaml#L3-L5(this comment)configs/model/virchow2_hybrid_mil_model.yaml#L3-L5🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@configs/model/pgp_hybrid_mil_model.yaml` around lines 3 - 5, Bind the required lr, tl_threshold, and sl_threshold parameters for ProstateCancerAttentionMIL using the shared experiment/default configuration or explicit valid overrides. Apply the same change to configs/model/pgp_hybrid_mil_model.yaml lines 3-5 and configs/model/virchow2_hybrid_mil_model.yaml lines 3-5, removing the mandatory ??? placeholders so Hydra can instantiate both models.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
prostate_cancer/callbacks/curves_callback_base.py (1)
119-135: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winHandle empty evaluation buffers before concatenation.
Both shared bases assume that every test or prediction epoch collects usable data. Empty loaders can make
torch.cat()ornp.concatenate()fail. Add an empty-data path before concatenation, and keep a separate mismatch error when only one buffer is populated.
prostate_cancer/callbacks/curves_callback_base.py#L119-L135: guard empty or zero-sample curve buffers before calling_plot_roc()and_plot_precision_recall().prostate_cancer/callbacks/histograms_callback_base.py#L19-L23: guard empty histogram buffers before callingnp.concatenate().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@prostate_cancer/callbacks/curves_callback_base.py` around lines 119 - 135, Update _plot_and_clear in prostate_cancer/callbacks/curves_callback_base.py at lines 119-135 to detect empty or zero-sample preds and targets before torch.cat(), return without plotting when both are empty, and raise a separate mismatch error when only one buffer is populated; retain clearing behavior as appropriate. Update the histogram buffer handling in prostate_cancer/callbacks/histograms_callback_base.py at lines 19-23 to guard empty buffers before np.concatenate().prostate_cancer/callbacks/curves_callback_sl.py (1)
20-28: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winReject a missing
Trainer.datamodulevalue, not only a missing attribute.PyTorch Lightning can expose
trainer.datamodulewithNone, sohasattr()can pass andtrainer.datamodule.predict.slidescan still fail. Read the value withgetattr(), rejectNone, then cast it.Proposed fix
- if not hasattr(trainer, "datamodule"): + datamodule = getattr(trainer, "datamodule", None) + if datamodule is None: raise ValueError("Trainer should have datamodule attribute") - datamodule = cast("BagOfTilesDataModule", trainer.datamodule) + datamodule = cast("BagOfTilesDataModule", datamodule)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@prostate_cancer/callbacks/curves_callback_sl.py` around lines 20 - 28, Update setup in the callback to retrieve trainer.datamodule via getattr with a None default, reject a missing or None value with the existing ValueError, and only then cast it to BagOfTilesDataModule before accessing predict.slides.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@prostate_cancer/callbacks/curves_callback_base.py`:
- Around line 119-135: Update _plot_and_clear in
prostate_cancer/callbacks/curves_callback_base.py at lines 119-135 to detect
empty or zero-sample preds and targets before torch.cat(), return without
plotting when both are empty, and raise a separate mismatch error when only one
buffer is populated; retain clearing behavior as appropriate. Update the
histogram buffer handling in
prostate_cancer/callbacks/histograms_callback_base.py at lines 19-23 to guard
empty buffers before np.concatenate().
In `@prostate_cancer/callbacks/curves_callback_sl.py`:
- Around line 20-28: Update setup in the callback to retrieve trainer.datamodule
via getattr with a None default, reject a missing or None value with the
existing ValueError, and only then cast it to BagOfTilesDataModule before
accessing predict.slides.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5cfc05bb-4c2c-4986-b043-f996d4169e5a
📒 Files selected for processing (10)
prostate_cancer/attention_mil_model.pyprostate_cancer/callbacks/curves_callback_base.pyprostate_cancer/callbacks/curves_callback_mil.pyprostate_cancer/callbacks/curves_callback_sl.pyprostate_cancer/callbacks/curves_callback_tl.pyprostate_cancer/callbacks/histograms_callback_base.pyprostate_cancer/callbacks/mil_prediction_callback.pyprostate_cancer/callbacks/slide_histograms_callback_mil.pyprostate_cancer/callbacks/tile_histograms_callback_mil.pyprostate_cancer/callbacks/tile_histograms_callback_tl.py
🚧 Files skipped from review as they are similar to previous changes (4)
- prostate_cancer/attention_mil_model.py
- prostate_cancer/callbacks/slide_histograms_callback_mil.py
- prostate_cancer/callbacks/mil_prediction_callback.py
- prostate_cancer/callbacks/tile_histograms_callback_mil.py
This PR adds MIL-based callbacks and evaluations.
Summary by CodeRabbit