feat: add provgigapath model#13
Conversation
📝 WalkthroughWalkthroughA new YAML configuration file for preprocessing embeddings is added to the experiment configuration directory. The file defines the prov-gigapath model with global package scope and Hydra-style defaults that map the dataset path to ChangesProv-GigaPath Embedding Configuration
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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.
Code Review
This pull request introduces a new configuration file for preprocessing embeddings using the prov-gigapath model. Feedback was provided regarding a naming inconsistency between the model identifier and the filename, as well as a missing trailing newline in the configuration file.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
configs/experiment/preprocessing/embeddings_prov_gigapath_05mpp.yaml (1)
1-7: 💤 Low valueConsider adding explicit resolution parameter for consistency with other preprocessing configs.
The filename includes
05mpp(0.5 microns per pixel), but this resolution is not explicitly set in the configuration. While other preprocessing configs (tiling_05mpp.yaml,tissue_masks_mpp2.yaml, etc.) explicitly define mpp parameters, the embedding configs use only the model name. Adding an explicitmpp: 0.5would improve clarity and maintain consistency across preprocessing configurations.🤖 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/experiment/preprocessing/embeddings_prov_gigapath_05mpp.yaml` around lines 1 - 7, The config is missing an explicit resolution setting; update the YAML by adding an mpp: 0.5 entry to this preprocessing config so it matches other files that declare microns-per-pixel explicitly (e.g., alongside the existing defaults block or near model: prov-gigapath) to make resolution unambiguous and consistent with tiling_05mpp and related configs.
🤖 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.
Nitpick comments:
In `@configs/experiment/preprocessing/embeddings_prov_gigapath_05mpp.yaml`:
- Around line 1-7: The config is missing an explicit resolution setting; update
the YAML by adding an mpp: 0.5 entry to this preprocessing config so it matches
other files that declare microns-per-pixel explicitly (e.g., alongside the
existing defaults block or near model: prov-gigapath) to make resolution
unambiguous and consistent with tiling_05mpp and related configs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 824c3782-6424-4b0e-b5e9-ff01245c1de7
📒 Files selected for processing (1)
configs/experiment/preprocessing/embeddings_prov_gigapath_05mpp.yaml
Adds the config for running the provgigapath using the new SDK.
Summary by CodeRabbit
Chores