feat: datamodule#2
Open
xrusnack wants to merge 66 commits into
Open
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the data handling capabilities of the Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a stage-aware Lightning DataModule, sampling utilities, batching/filtering helpers, typed data schemas, and YAML configurations to support slide-level supervision, weighted sampling by slide positivity, and dataloader/collation for training, testing, and prediction workflows. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DataModule
participant MLflow
participant Dataset as NucleiDataset
participant Sampler as AutoWeightedRandomSampler
participant Collator
User->>DataModule: setup(stage="fit")
DataModule->>MLflow: download metadata & supervision parquet
MLflow-->>DataModule: metadata DataFrames
DataModule->>DataModule: train_test_split (stratify/group if set)
DataModule->>DataModule: apply min_count / min_positive filters
DataModule->>DataModule: build supervision (train_strategy)
DataModule->>Dataset: instantiate train/val datasets (full_slide per stage)
User->>DataModule: train_dataloader()
DataModule->>Sampler: init with dataset + slides_positivity + params
Sampler->>Sampler: compute per-sample weights
DataModule->>DataModule: create DataLoader (sampler/collate_fn)
DataModule-->>User: return DataLoader
loop Training loop
User->>DataModule: request next batch
DataModule->>Dataset: get samples
DataModule->>Collator: collate_fn(batch)
Collator-->>DataModule: batched tensors
DataModule-->>User: yield batch
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive LightningDataModule for handling nuclei graph data, including data loading, supervision strategies, and sampling. The implementation is well-structured with clear separation of concerns into different modules for data handling, sampling, and utilities. My review focuses on improving robustness, fixing a minor formatting bug, and enhancing clarity in type definitions. Overall, this is a solid feature addition.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
nuclei_graph/data/samplers/weighted_random_sampler.py (1)
37-48: Harden weight computation for missing slide IDs and degenerate class splits.Line 40 assumes full key coverage in
slides_positivity. Adding explicit validation and handling single-class cases makes failures clearer and behavior more predictable.Proposed robustness patch
) -> Sequence[float]: slide_ids = dataset.slides["slide_id"].values + missing = [sid for sid in slide_ids if sid not in slides_positivity] + if missing: + raise ValueError( + f"Missing positivity scores for {len(missing)} slide(s), e.g. {missing[:5]}" + ) + labels = torch.tensor( [ 1.0 if slides_positivity[slide_id] > positivity_thr else 0.0 for slide_id in slide_ids ] ) - positive = labels.sum() - negative = len(labels) - positive + positive = int(labels.sum().item()) + negative = len(labels) - positive + if positive == 0 or negative == 0: + return torch.ones(len(labels), dtype=torch.float32).tolist() + weights = torch.zeros_like(labels) weights[labels == 0] = 1.0 / negative weights[labels == 1] = 1.0 / positive return weights.tolist()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nuclei_graph/data/samplers/weighted_random_sampler.py` around lines 37 - 48, Validate that every slide_id in slide_ids exists in slides_positivity before computing labels and handle missing keys explicitly (either by raising a clear error referencing slides_positivity and slide_id or by assigning a default positivity value). After computing labels (using slides_positivity and positivity_thr), guard the subsequent class counts positive and negative against zero to avoid division by zero: if one class is absent, assign sensible fallback weights (e.g., uniform weights or all mass to the present class) instead of dividing by zero. Ensure these checks are applied around the computations of labels, positive, negative, and weights so weights[labels==0] and weights[labels==1] are always safe to compute.nuclei_graph/data/utils/filtering.py (1)
14-17: Use structured logging instead ofLines 14-17 currently print directly; switching to a logger makes this controllable in training/inference environments.
Proposed refactor
import pandas as pd +import logging + +logger = logging.getLogger(__name__) @@ - print( - f"[INFO] Dropped slides with < {min_count} nuclei:\n", - dropped_slides.to_string(index=False), - ) + logger.info( + "Dropped slides with < %s nuclei:\n%s", + min_count, + dropped_slides.to_string(index=False), + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nuclei_graph/data/utils/filtering.py` around lines 14 - 17, Replace the direct print call that reports dropped slides with structured logging: add a module-level logger (e.g., logger = logging.getLogger(__name__)) and change the print(...) to logger.info(...) using a formatted message and parameters (include min_count and dropped_slides.to_string(index=False) in the log). Ensure the logging module is imported and that the log message is concise and structured rather than using print, so dropped_slides and min_count are emitted via logger.info instead of print.nuclei_graph/data/data_module.py (1)
148-151: Prefer explicit runtime exceptions overassertfor data/config validation.Lines 148, 151, and 189 are runtime contract checks; explicit exceptions keep behavior consistent even when assertions are disabled.
Proposed refactor
- assert self.train_strategy is not None + if self.train_strategy is None: + raise ValueError("train_strategy must be set for stage='fit'/'validate'.") @@ - assert slides_df is not None + if slides_df is None: + raise ValueError(f"Failed to load training metadata from URI: {slides_uri}") @@ - assert slides_df is not None + if slides_df is None: + raise ValueError(f"Failed to load metadata for stage='{stage}' from URI: {slides_uri}")Also applies to: 189-189
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nuclei_graph/data/data_module.py` around lines 148 - 151, Replace the runtime asserts with explicit exceptions: instead of "assert self.train_strategy is not None" raise a clear error (e.g. ValueError or RuntimeError) indicating that train_strategy must be configured; instead of "assert slides_df is not None" raise a descriptive exception (e.g. FileNotFoundError or ValueError) that includes slides_uri and notes the failure of _load_df to return data; and apply the same pattern to the other runtime contract assert in this module (the assert referenced at line 189) so all validation failures use explicit exceptions with helpful messages.configs/ml.yaml (1)
4-4: Prefer a functional selector over inline profile comments.Line 4’s
# crop_level nuclei_levelcomment is easy to stale. Consider a dedicated selector variable (or documented override example) instead of a free-form inline note.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@configs/ml.yaml` at line 4, Replace the inline comment on the /experiment/modeling/training entry with a dedicated selector variable (e.g., add a new key like training_selector or training_mode) and document allowed values (crop_level, nuclei_level) so callers can override explicitly; update any code reading /experiment/modeling/training to prefer the new selector (training_selector or training_mode) and fall back to the existing value only if the selector is absent so behavior remains backward-compatible.configs/experiment/modeling/training/crop_level.yaml (1)
3-27: Consider extracting a shared training template.This file is almost identical to
nuclei_level.yamlexcept metadata naming; a shared base template would reduce drift and maintenance overhead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@configs/experiment/modeling/training/crop_level.yaml` around lines 3 - 27, This config largely duplicates another config (e.g., identical defaults and trainer/data/model blocks) — extract the common pieces into a shared base config (e.g., experiment/modeling/training/shared_base.yaml) and have crop_level.yaml and nuclei_level.yaml include that base via the defaults list; move repeated keys (defaults, trainer, data.sampler_pos_thr, balance_sampling, batch_size, split_size, model) into the shared base and keep only overrides in crop_level.yaml (e.g., metadata.run_name and any true diffs), updating the defaults entry instead of duplicating full blocks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@configs/data/samplers/weighted_random_sampler.yaml`:
- Line 3: The sampler config contains an unresolved required Hydra value
`positivity_thr: ???` which causes MissingMandatoryValue; replace the
placeholder by wiring it to the experiments' parameter (e.g., set
`positivity_thr: ${data.sampler_pos_thr}` in the sampler config) or
alternatively explicitly set `positivity_thr` in each experiment config
(`nuclei_level.yaml` and `crop_level.yaml`) so the sampler sees a concrete
float; target the `positivity_thr` key in the sampler config or the
`data.sampler_pos_thr` keys in the experiment configs when making the change.
In `@configs/experiment/modeling/training/base.yaml`:
- Around line 19-22: The base hyperparameters use null-safe oc.select for
sampler_positivity_threshold but not for batch_size, split_size, and
balance_sampling; update the interpolations for batch_size, split_size, and
balance_sampling to use the same null-safe pattern (oc.select) so all four
keys—sampler_positivity_threshold, batch_size, split_size, balance_sampling—are
resolved via ${oc.select:... , null} to avoid interpolation errors when composed
configs omit them.
In `@configs/experiment/modeling/training/crop_level.yaml`:
- Around line 6-12: The YAML uses explicit nulls that override defaults composed
earlier — when _self_ is used the entries like /data/datasets@data.dataset and
/model/meta_archs@model (and the nulls at data.dataset and model later) are
being set to null instead of deferring to the composed defaults; remove the
explicit nulls or replace them with Hydra delete syntax (~data.dataset, ~model)
if you intend to remove those keys, and ensure _self_ remains last so default
selections from /model/meta_archs@model, /model/models@model.net,
/data/datasets@data.dataset, /data/samplers@data.sampler, and
/data/supervision@data.train_strategy|data.eval_strategy are not overwritten by
null values.
In `@configs/experiment/modeling/training/nuclei_level.yaml`:
- Around line 6-12: The YAML composition places `_self_` last causing null
entries for `model` and `data.dataset` to override earlier composed defaults;
move or merge `_self_` so that entries from `/model/meta_archs@model`,
`/model/models@model.net`, and `/data/datasets@data.dataset` are not wiped.
Concretely, ensure `_self_` appears before default includes or remove the
explicit null keys under `model` and `data.dataset` in this file so that the
composed values from those selectors are preserved (check references
`/model/meta_archs@model`, `/model/models@model.net`,
`/data/datasets@data.dataset`, and the `_self_` anchor).
In `@nuclei_graph/data/data_module.py`:
- Around line 163-175: After calling min_count_filter on train_df, add a
fail-fast check immediately after that line to detect an empty train_df and
raise a clear exception (or ValueError) that explains that min_count_filter
removed all training slides and includes the dataset_cfg/crop_size context; this
should occur before calling _load_supervision_dfs, _prepare_supervision, or
instantiate so train_sup and train_dataset are not constructed from an empty
DataFrame.
In `@nuclei_graph/data/utils/collator.py`:
- Around line 18-21: The current code builds graph_targets by filtering out None
which breaks per-sample alignment; update the collator logic in collator.py
(variables graph_targets and batched_y construction) to first collect
graph_entries = [b["y"]["graph"] for b in batch] and then enforce all-or-none
graph supervision: if any(entry is None for entry in graph_entries) and not
all(entry is None for entry in graph_entries) raise a ValueError, if all are
None set batched_y["graph"]=None, otherwise set
batched_y["graph"]=torch.stack(graph_entries, dim=0) so ordering and batch
alignment are preserved for the graph target.
---
Nitpick comments:
In `@configs/experiment/modeling/training/crop_level.yaml`:
- Around line 3-27: This config largely duplicates another config (e.g.,
identical defaults and trainer/data/model blocks) — extract the common pieces
into a shared base config (e.g., experiment/modeling/training/shared_base.yaml)
and have crop_level.yaml and nuclei_level.yaml include that base via the
defaults list; move repeated keys (defaults, trainer, data.sampler_pos_thr,
balance_sampling, batch_size, split_size, model) into the shared base and keep
only overrides in crop_level.yaml (e.g., metadata.run_name and any true diffs),
updating the defaults entry instead of duplicating full blocks.
In `@configs/ml.yaml`:
- Line 4: Replace the inline comment on the /experiment/modeling/training entry
with a dedicated selector variable (e.g., add a new key like training_selector
or training_mode) and document allowed values (crop_level, nuclei_level) so
callers can override explicitly; update any code reading
/experiment/modeling/training to prefer the new selector (training_selector or
training_mode) and fall back to the existing value only if the selector is
absent so behavior remains backward-compatible.
In `@nuclei_graph/data/data_module.py`:
- Around line 148-151: Replace the runtime asserts with explicit exceptions:
instead of "assert self.train_strategy is not None" raise a clear error (e.g.
ValueError or RuntimeError) indicating that train_strategy must be configured;
instead of "assert slides_df is not None" raise a descriptive exception (e.g.
FileNotFoundError or ValueError) that includes slides_uri and notes the failure
of _load_df to return data; and apply the same pattern to the other runtime
contract assert in this module (the assert referenced at line 189) so all
validation failures use explicit exceptions with helpful messages.
In `@nuclei_graph/data/samplers/weighted_random_sampler.py`:
- Around line 37-48: Validate that every slide_id in slide_ids exists in
slides_positivity before computing labels and handle missing keys explicitly
(either by raising a clear error referencing slides_positivity and slide_id or
by assigning a default positivity value). After computing labels (using
slides_positivity and positivity_thr), guard the subsequent class counts
positive and negative against zero to avoid division by zero: if one class is
absent, assign sensible fallback weights (e.g., uniform weights or all mass to
the present class) instead of dividing by zero. Ensure these checks are applied
around the computations of labels, positive, negative, and weights so
weights[labels==0] and weights[labels==1] are always safe to compute.
In `@nuclei_graph/data/utils/filtering.py`:
- Around line 14-17: Replace the direct print call that reports dropped slides
with structured logging: add a module-level logger (e.g., logger =
logging.getLogger(__name__)) and change the print(...) to logger.info(...) using
a formatted message and parameters (include min_count and
dropped_slides.to_string(index=False) in the log). Ensure the logging module is
imported and that the log message is concise and structured rather than using
print, so dropped_slides and min_count are emitted via logger.info instead of
print.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0050cd6e-46c6-4045-be66-019f3c0c9170
📒 Files selected for processing (12)
configs/data/samplers/weighted_random_sampler.yamlconfigs/experiment/modeling/training/base.yamlconfigs/experiment/modeling/training/crop_level.yamlconfigs/experiment/modeling/training/nuclei_level.yamlconfigs/ml.yamlnuclei_graph/data/data_module.pynuclei_graph/data/samplers/__init__.pynuclei_graph/data/samplers/weighted_random_sampler.pynuclei_graph/data/utils/__init__.pynuclei_graph/data/utils/collator.pynuclei_graph/data/utils/filtering.pynuclei_graph/nuclei_graph_typing.py
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
nuclei_graph/data/data_module.py (1)
138-151:⚠️ Potential issue | 🔴 CriticalFail fast when filtering removes the training split.
Line 138 and Line 149 can both reduce
train_dfto zero rows. The code then continues into supervision/dataset construction, anddrop_last=Truelater turns that into a zero-batch epoch instead of a clear setup error.🚨 Proposed guard
train_df = min_count_filter(train_df, self.dataset_cfg.crop_size) + if train_df.empty: + raise ValueError( + "No training slides remain after min_count_filter; " + f"crop_size={self.dataset_cfg.crop_size}." + ) train_sup_dfs = self._load_sup_sources(self.train_strategy) train_sup = self._prepare_supervision( train_df, train_sup_dfs, self.train_strategy ) self.positivity = train_sup.positivity_map if self.dataset_cfg.mil: min_pos_count = ( self.dataset_cfg.crop_size * self.dataset_cfg.crop_pos_thr ) train_df = min_positive_count_filter( train_df, min_pos_count, train_sup.pos_count_map ) + if train_df.empty: + raise ValueError( + "No training slides remain after min_positive_count_filter; " + f"min_pos_count={min_pos_count}." + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nuclei_graph/data/data_module.py` around lines 138 - 151, After each filtering step that can drop all training rows (after min_count_filter(train_df, self.dataset_cfg.crop_size) and after min_positive_count_filter(...)), immediately check if train_df is empty and raise a clear exception (e.g., ValueError) describing which filter removed the split and include relevant context (dataset_cfg.crop_size, dataset_cfg.crop_pos_thr, and train_strategy); do this before calling _load_sup_sources, _prepare_supervision or using train_sup/positivity_map so the pipeline fails fast instead of producing zero-batch epochs.
🧹 Nitpick comments (1)
nuclei_graph/data/samplers/weighted_random_sampler.py (1)
18-27: Document the actual weighting behavior.This sampler is not doing plain inverse-frequency weighting, and
replacementis not optional in the current signature. The docstring should describe thepos_slide_ratiocontract explicitly so experiment configs are tuned against the real behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nuclei_graph/data/samplers/weighted_random_sampler.py` around lines 18 - 27, Update the class/function docstring for WeightedRandomSampler (and its __init__ signature) to state the exact weighting behavior used (referencing the internal method that computes weights, e.g., any compute_weights or weight assignment logic) — explicitly describe the formula (for example: per-sample weight = 1 / freq(class) or if you normalize across slides or use slide-level positivity to scale sample weights, state that exact computation), clarify that replacement is currently required/not optional in the implementation (match the actual flag behavior in __init__), and spell out the pos_slide_ratio contract precisely (what range it accepts, whether it is applied to slide-level selection vs sample-level weighting, and whether it is a hard quota or target ratio). Make the docstring changes in WeightedRandomSampler and its __init__ so experiment configs can be tuned to the real behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nuclei_graph/data/samplers/weighted_random_sampler.py`:
- Around line 42-50: Validate pos_slide_ratio is within [0,1] and guard the
divisions by ensuring both class counts (variables positive and negative
computed from labels) are greater than zero before computing weights; if
positive==0 or negative==0 or pos_slide_ratio is out of range, raise a clear
ValueError describing the bad configuration (mentioning pos_slide_ratio,
positive, negative, slide_ids/slides_positivity) instead of proceeding to
compute weights[labels == 0] and weights[labels == 1]; update the logic around
labels, positive, negative, pos_slide_ratio and weights to perform these checks
early and fail fast with a descriptive message.
---
Duplicate comments:
In `@nuclei_graph/data/data_module.py`:
- Around line 138-151: After each filtering step that can drop all training rows
(after min_count_filter(train_df, self.dataset_cfg.crop_size) and after
min_positive_count_filter(...)), immediately check if train_df is empty and
raise a clear exception (e.g., ValueError) describing which filter removed the
split and include relevant context (dataset_cfg.crop_size,
dataset_cfg.crop_pos_thr, and train_strategy); do this before calling
_load_sup_sources, _prepare_supervision or using train_sup/positivity_map so the
pipeline fails fast instead of producing zero-batch epochs.
---
Nitpick comments:
In `@nuclei_graph/data/samplers/weighted_random_sampler.py`:
- Around line 18-27: Update the class/function docstring for
WeightedRandomSampler (and its __init__ signature) to state the exact weighting
behavior used (referencing the internal method that computes weights, e.g., any
compute_weights or weight assignment logic) — explicitly describe the formula
(for example: per-sample weight = 1 / freq(class) or if you normalize across
slides or use slide-level positivity to scale sample weights, state that exact
computation), clarify that replacement is currently required/not optional in the
implementation (match the actual flag behavior in __init__), and spell out the
pos_slide_ratio contract precisely (what range it accepts, whether it is applied
to slide-level selection vs sample-level weighting, and whether it is a hard
quota or target ratio). Make the docstring changes in WeightedRandomSampler and
its __init__ so experiment configs can be tuned to the real behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cf30dd6d-015e-4f26-ae76-88d2ddb2cd1a
📒 Files selected for processing (10)
configs/data/samplers/weighted_random_sampler.yamlconfigs/experiment/modeling/training/base.yamlconfigs/experiment/modeling/training/crop_level.yamlconfigs/experiment/modeling/training/nuclei_level.yamlconfigs/ml.yamlnuclei_graph/data/data_module.pynuclei_graph/data/samplers/weighted_random_sampler.pynuclei_graph/data/utils/__init__.pynuclei_graph/data/utils/collator.pynuclei_graph/data/utils/filtering.py
✅ Files skipped from review due to trivial changes (3)
- configs/data/samplers/weighted_random_sampler.yaml
- configs/experiment/modeling/training/nuclei_level.yaml
- configs/experiment/modeling/training/crop_level.yaml
🚧 Files skipped from review as they are similar to previous changes (5)
- configs/ml.yaml
- nuclei_graph/data/utils/init.py
- configs/experiment/modeling/training/base.yaml
- nuclei_graph/data/utils/collator.py
- nuclei_graph/data/utils/filtering.py
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
visualization/polygons2raster.py (1)
148-151: Consider specifyingkeepparameter indrop_duplicatesfor clarity.The
drop_duplicates(subset=["slide_path"])call defaults tokeep='first', which may silently discard rows if the same slide appears in multiple metadata URIs with differentslide_nuclei_pathvalues. If this is the intended behavior, adding an explicitkeep='first'would improve readability. Alternatively, if duplicates are unexpected, consider raising an error or logging a warning.💡 Optional: Make deduplication behavior explicit
def uris2df(uris: list[str]) -> pd.DataFrame: """Loads and merges multiple metadata Parquet files into a single DataFrame.""" batches = [pd.read_parquet(download_artifacts(uri)) for uri in uris] - return pd.concat(batches, ignore_index=True).drop_duplicates(subset=["slide_path"]) + return pd.concat(batches, ignore_index=True).drop_duplicates(subset=["slide_path"], keep="first")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@visualization/polygons2raster.py` around lines 148 - 151, The uris2df function currently calls drop_duplicates(subset=["slide_path"]) which relies on the default keep='first' implicitly; update uris2df to make deduplication behavior explicit by specifying keep='first' if keeping the first occurrence is intended, or change to keep='last' as appropriate, or instead detect duplicate slide_path values after concatenation and either raise an exception or log a warning (using the project logger) so mismatched slide_nuclei_path values are surfaced; refer to the uris2df function and its drop_duplicates call to implement the chosen explicit behavior.preprocessing/data_split.py (1)
31-33: Consider using.locor.copy()to avoid potentialSettingWithCopyWarning.Assigning to
train["set"]andtest["set"]directly may trigger aSettingWithCopyWarningdepending on the pandas version and howtrain_test_splitreturns the DataFrames. Using.locor explicitly calling.copy()on the split results would make the intent clearer.💡 Optional: Explicit copy to avoid warnings
train, test = train_test_split( slides, test_size=config.test_size, random_state=42, stratify=slides[config.stratify_column], ) - train["set"] = "train" - test["set"] = "test" + train = train.copy() + test = test.copy() + train["set"] = "train" + test["set"] = "test" split = pd.concat([train, test], ignore_index=True)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@preprocessing/data_split.py` around lines 31 - 33, The direct assignments train["set"] = "train" and test["set"] = "test" can trigger pandas SettingWithCopyWarning; to fix, make the split DataFrames explicit copies (e.g., call copy() on the results returned into train and test) or assign via .loc (e.g., train.loc[:, "set"] = "train" and test.loc[:, "set"] = "test") before concatenating into split so the operations modify bona fide DataFrames rather than potential views.exploration/panda/save_metadataset.py (1)
34-37: Serialize error logging instead of appending from every worker.All Ray tasks share the same
errors.logpath and write to it concurrently. That can interleave or lose log lines under parallel execution, especially on network-backed storage. Prefer collecting error strings in the driver or routing writes through a dedicated actor.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@exploration/panda/save_metadataset.py` around lines 34 - 37, The current log function (def log(msg: str)) appends directly to the shared errors.log from every Ray worker causing interleaved/lost lines; change workers to stop writing files directly and either (a) return error strings from the Ray tasks to the driver and perform single-threaded writes to errors.log in the driver, or (b) create a dedicated Ray actor (e.g., class ErrorLogger with a log(self, msg: str) method) that serializes writes and call error_logger.log.remote(msg) from workers; update all usages of log(...) in the code to instead return errors or call the actor so only one process performs file I/O.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@exploration/panda/save_metadataset.py`:
- Around line 51-60: The current use of tifffile.imread(materializes the whole
raster) should be replaced by a lightweight validation: open the file with
tifffile.TiffFile in a context manager (instead of tifffile.imread), check that
the file has at least one page/series and that the first page/series reports a
non-zero shape/size (e.g., pages or series metadata) without calling .asarray()
or reading pixel data, and preserve the same exception handling and log messages
(referencing mask_path, tifffile.TiffFile, and log). If the file has no pages or
reports zero pixels treat it as MASK_EMPTY and return False; on any exception
log MASK_CORRUPTED with the exception string and return False.
In `@preprocessing/metadata_mapping/panda.py`:
- Around line 35-45: The lambda in nuclei_paths is using parameter name "id"
which shadows Python's built-in id(); change the parameter to a descriptive name
(e.g., "sid") in the nuclei_paths map call (the expression nuclei_paths =
slides["slide_id"].map(lambda id: nuclei_dir / f"slide_id={id}")) and update any
corresponding references in that lambda to use the new name so the behavior of
nuclei_paths and subsequent map_df entries ("slide_id", "slide_nuclei_path")
remains unchanged.
---
Nitpick comments:
In `@exploration/panda/save_metadataset.py`:
- Around line 34-37: The current log function (def log(msg: str)) appends
directly to the shared errors.log from every Ray worker causing interleaved/lost
lines; change workers to stop writing files directly and either (a) return error
strings from the Ray tasks to the driver and perform single-threaded writes to
errors.log in the driver, or (b) create a dedicated Ray actor (e.g., class
ErrorLogger with a log(self, msg: str) method) that serializes writes and call
error_logger.log.remote(msg) from workers; update all usages of log(...) in the
code to instead return errors or call the actor so only one process performs
file I/O.
In `@preprocessing/data_split.py`:
- Around line 31-33: The direct assignments train["set"] = "train" and
test["set"] = "test" can trigger pandas SettingWithCopyWarning; to fix, make the
split DataFrames explicit copies (e.g., call copy() on the results returned into
train and test) or assign via .loc (e.g., train.loc[:, "set"] = "train" and
test.loc[:, "set"] = "test") before concatenating into split so the operations
modify bona fide DataFrames rather than potential views.
In `@visualization/polygons2raster.py`:
- Around line 148-151: The uris2df function currently calls
drop_duplicates(subset=["slide_path"]) which relies on the default keep='first'
implicitly; update uris2df to make deduplication behavior explicit by specifying
keep='first' if keeping the first occurrence is intended, or change to
keep='last' as appropriate, or instead detect duplicate slide_path values after
concatenation and either raise an exception or log a warning (using the project
logger) so mismatched slide_nuclei_path values are surfaced; refer to the
uris2df function and its drop_duplicates call to implement the chosen explicit
behavior.
🪄 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
Run ID: b0e9e280-5666-41f6-9ff5-39b591a1f7ef
📒 Files selected for processing (45)
configs/base.yamlconfigs/data/sources/panda.yamlconfigs/data/sources/prostate_cancer.yamlconfigs/data/sources/prostate_cancer_mmci_tl.yamlconfigs/experiment/preprocessing/annotation_labels/panda.yamlconfigs/experiment/preprocessing/cam_labels/annot_restricted_thr/prostate_cancer_mmci_tl.yamlconfigs/experiment/preprocessing/cam_labels/default_thr/prostate_cancer_mmci_tl.yamlconfigs/experiment/preprocessing/cam_masks/prostate_cancer_mmci_tl.yamlconfigs/experiment/preprocessing/data_split/radboud.yamlconfigs/experiment/preprocessing/metadata_mapping/radboud.yamlconfigs/experiment/preprocessing/unipolar_heatmap_labels/annotation/prostate_cancer_mmci_tl.yamlconfigs/experiment/preprocessing/unipolar_heatmap_labels/prediction/virchow2/prostate_cancer_mmci_tl.yamlconfigs/experiment/visualization/annotation_labels.yamlconfigs/exploration/panda/save_metadataset.yamlconfigs/exploration/prostate_cancer_mmci_tl/save_metadataset.yamlconfigs/preprocessing/annotation_labels.yamlconfigs/preprocessing/data_split.yamlconfigs/preprocessing/metadata_mapping/panda.yamlconfigs/preprocessing/metadata_mapping/prostate_cancer_mmci_tl.yamlconfigs/preprocessing/nuclei_standardization.yamlconfigs/visualization/polygons2raster/prostate_cancer_mmci_tl.yamlconfigs/visualization/polygons2raster/radboud.yamlexploration/__init__.pyexploration/panda/__init__.pyexploration/panda/save_metadataset.pyexploration/prostate_cancer_mmci_tl/__init__.pyexploration/prostate_cancer_mmci_tl/save_metadataset.pypreprocessing/__init__.pypreprocessing/annotation_labels.pypreprocessing/data_split.pypreprocessing/metadata_mapping/__init__.pypreprocessing/metadata_mapping/panda.pypreprocessing/metadata_mapping/prostate_cancer_mmci_tl.pypreprocessing/nuclei_standardization.pyscripts/exploration/panda/__init__.pyscripts/exploration/panda/run_save_metadataset.pyscripts/exploration/prostate_cancer_mmci_tl/__init__.pyscripts/exploration/prostate_cancer_mmci_tl/run_save_metadataset.pyscripts/preprocessing/metadata_mapping/run_panda.pyscripts/preprocessing/metadata_mapping/run_prostate_cancer_mmci_tl.pyscripts/preprocessing/run_annotation_labels.pyscripts/preprocessing/run_data_split.pyscripts/preprocessing/run_nuclei_standardization.pyscripts/visualization/run_polygons_rasterization.pyvisualization/polygons2raster.py
💤 Files with no reviewable changes (2)
- configs/experiment/visualization/annotation_labels.yaml
- configs/data/sources/prostate_cancer.yaml
✅ Files skipped from review due to trivial changes (16)
- configs/experiment/preprocessing/unipolar_heatmap_labels/prediction/virchow2/prostate_cancer_mmci_tl.yaml
- configs/experiment/preprocessing/cam_masks/prostate_cancer_mmci_tl.yaml
- configs/experiment/preprocessing/cam_labels/annot_restricted_thr/prostate_cancer_mmci_tl.yaml
- configs/experiment/preprocessing/cam_labels/default_thr/prostate_cancer_mmci_tl.yaml
- configs/experiment/preprocessing/unipolar_heatmap_labels/annotation/prostate_cancer_mmci_tl.yaml
- configs/experiment/preprocessing/metadata_mapping/radboud.yaml
- configs/data/sources/panda.yaml
- configs/preprocessing/nuclei_standardization.yaml
- configs/experiment/preprocessing/data_split/radboud.yaml
- configs/base.yaml
- configs/experiment/preprocessing/annotation_labels/panda.yaml
- configs/exploration/panda/save_metadataset.yaml
- configs/preprocessing/data_split.yaml
- configs/visualization/polygons2raster/radboud.yaml
- configs/preprocessing/metadata_mapping/panda.yaml
- configs/data/sources/prostate_cancer_mmci_tl.yaml
There was a problem hiding this comment.
🧹 Nitpick comments (2)
nuclei_graph/data/data_module.py (2)
133-143: Consider makingrandom_stateconfigurable.The train/val split seed is hardcoded to
42. For reproducibility control and for sweeps/ablations where a different seed is needed, exposing this as a constructor/config argument (e.g.,split_seed: int = 42) is generally preferable and aligns with the other split-related parameters already plumbed through (split_stratify_col,split_group_col,split_size).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nuclei_graph/data/data_module.py` around lines 133 - 143, Add a configurable seed parameter (e.g., split_seed: int = 42) to the DataModule constructor and store it as self.split_seed, then replace the hardcoded random_state=42 in the train_test_split call with random_state=self.split_seed; update any docstring/parameter list for the class to document split_seed and ensure train_test_split (the call using slides_df, self.split_size, self.split_stratify_col, and self.split_group_col) uses that stored value for reproducible and configurable splits.
227-245: Inconsistentpin_memoryacross eval dataloaders.
val_dataloadersetspin_memory=Truebuttest_dataloaderandpredict_dataloaderdo not. If the omission is deliberate, ignore; otherwise align them for consistent host→device transfer behavior.Proposed alignment
def test_dataloader(self) -> Iterable[Batch]: return DataLoader( self.test_dataset, batch_size=1, num_workers=self.eval_num_workers, persistent_workers=self.eval_num_workers > 0, prefetch_factor=2 if self.eval_num_workers > 0 else None, + pin_memory=True, collate_fn=collate_fn, ) def predict_dataloader(self) -> Iterable[PredictBatch]: return DataLoader( self.predict_dataset, batch_size=1, num_workers=self.eval_num_workers, persistent_workers=self.eval_num_workers > 0, prefetch_factor=2 if self.eval_num_workers > 0 else None, + pin_memory=True, collate_fn=collate_fn_predict, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nuclei_graph/data/data_module.py` around lines 227 - 245, The test_dataloader and predict_dataloader are missing the pin_memory setting present in val_dataloader; update both DataLoader constructions in test_dataloader and predict_dataloader to include pin_memory=True (or use the same eval pin_memory attribute if one exists) so host→device transfer behavior is consistent with val_dataloader.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@nuclei_graph/data/data_module.py`:
- Around line 133-143: Add a configurable seed parameter (e.g., split_seed: int
= 42) to the DataModule constructor and store it as self.split_seed, then
replace the hardcoded random_state=42 in the train_test_split call with
random_state=self.split_seed; update any docstring/parameter list for the class
to document split_seed and ensure train_test_split (the call using slides_df,
self.split_size, self.split_stratify_col, and self.split_group_col) uses that
stored value for reproducible and configurable splits.
- Around line 227-245: The test_dataloader and predict_dataloader are missing
the pin_memory setting present in val_dataloader; update both DataLoader
constructions in test_dataloader and predict_dataloader to include
pin_memory=True (or use the same eval pin_memory attribute if one exists) so
host→device transfer behavior is consistent with val_dataloader.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fe33de16-622f-4163-b791-35f74a9528ea
📒 Files selected for processing (4)
configs/experiment/modeling/training/base.yamlconfigs/ml.yamlnuclei_graph/data/data_module.pynuclei_graph/data/samplers/weighted_random_sampler.py
✅ Files skipped from review due to trivial changes (1)
- configs/experiment/modeling/training/base.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- configs/ml.yaml
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
nuclei_graph/data/data_module.py (1)
127-130: Use explicit runtime validation instead ofassert.These checks protect required configuration and loaded data. While assertions function during normal execution, they provide no actionable error messages and become problematic when Python is run with optimization flags (
-O), which strip assertions entirely. Replacing them with explicit exceptions makes failures more debuggable.Suggested fix
case "fit" | "validate": - assert self.split_size is not None + if self.split_size is None: + raise ValueError("`split_size` must be configured for fit/validate setup.") slides_df = self._load_df(slides_uri) - assert slides_df is not None + if slides_df is None: + raise ValueError("Training metadata URI must be configured for fit/validate setup.") train_df, validation_df = train_test_split( @@ if stage == "fit": - assert self.train_strategy is not None + if self.train_strategy is None: + raise ValueError("`supervision.train_strategy` must be configured for fit setup.")Also applies to: 146-148
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nuclei_graph/data/data_module.py` around lines 127 - 130, Replace the runtime assertions with explicit validation and informative exceptions: instead of using assert self.split_size is not None and assert slides_df is not None in the case "fit" | "validate" block (and the similar asserts at the later occurrence around lines 146-148), check the conditions and raise ValueError or RuntimeError with clear messages (e.g., "split_size must be set for fit/validate" and "failed to load slides dataframe from {slides_uri}") so failures are never stripped by -O and provide actionable diagnostics; locate these checks around the method that calls self._load_df and update them to validate self.split_size and the returned slides_df before proceeding.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nuclei_graph/data/data_module.py`:
- Around line 178-183: The code currently treats "test" and "predict" the same
by calling _load_df with METADATA_COLS_EVAL and then _prepare_supervision;
change this so "test" retains the supervised flow (keep _load_df(...,
cols=METADATA_COLS_EVAL) and call _prepare_supervision(slides_df,
self.eval_strategy.paths, self.eval_strategy)), while "predict" loads a
prediction-only metadata column set (e.g. METADATA_COLS_PRED or a subset that
omits is_carcinoma) via _load_df(slides_uri, cols=...) and does NOT call
_prepare_supervision; instead create the PredictBatch/Metadata using that
label-free slides_df (so PredictBatch uses Metadata without is_carcinoma).
Ensure slides_df is still checked for None in both branches and reference the
existing methods/objects: _load_df, METADATA_COLS_EVAL, _prepare_supervision,
PredictBatch, Metadata, and self.eval_strategy.
---
Nitpick comments:
In `@nuclei_graph/data/data_module.py`:
- Around line 127-130: Replace the runtime assertions with explicit validation
and informative exceptions: instead of using assert self.split_size is not None
and assert slides_df is not None in the case "fit" | "validate" block (and the
similar asserts at the later occurrence around lines 146-148), check the
conditions and raise ValueError or RuntimeError with clear messages (e.g.,
"split_size must be set for fit/validate" and "failed to load slides dataframe
from {slides_uri}") so failures are never stripped by -O and provide actionable
diagnostics; locate these checks around the method that calls self._load_df and
update them to validate self.split_size and the returned slides_df before
proceeding.
🪄 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
Run ID: 766271fc-9cfb-4066-89fc-14a992b6bd79
📒 Files selected for processing (1)
nuclei_graph/data/data_module.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit