random feedback improvements#189
Conversation
|
Need an answer fast? Review this PR in Change Stack to ask focused questions about the PR or a changed range. Warning Review limit reached
More reviews will be available in 4 minutes and 20 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughAligns CI branch triggers across workflows, updates e2e Python install and log messaging, fixes a SLURM echo typo, removes an unused amputation mask init, adjusts DiffWave residual-loop, sets training dataset defaults and renames test SHAP output, and updates README/CLI/loader/dependency strings. ChangesCodebase maintenance and pipeline updates
sequenceDiagram
participant GitHub
participant E2EIntegration
participant PythonRuntime
participant CodeQuality
GitHub->>CodeQuality: trigger push/pull_request (main, master, release/*, development)
GitHub->>E2EIntegration: trigger push/pull_request (main, master, release/*, development)
E2EIntegration->>PythonRuntime: run `uv python install ${{ matrix.python-version }}`
E2EIntegration->>E2EIntegration: verify log path `yaib_logs/mimic_demo`
🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
icu_benchmarks/models/train.py (1)
263-263: ⚡ Quick winConsider renaming for consistency.
The test SHAP values now use the pattern
test_shap_values.parquet(split prefix), while train SHAP values still useshap_values_train.parquet(split suffix). For consistency and maintainability, both should follow the same naming convention.📝 Proposed fix to align naming convention
- with (log_dir / "shap_values_train.parquet").open("wb") as f: + with (log_dir / "train_shap_values.parquet").open("wb") as f: shaps_train.write_parquet(f)🤖 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 `@icu_benchmarks/models/train.py` at line 263, Rename the train SHAP output filename to match the split-prefix convention used by tests (change "shap_values_train.parquet" to "train_shap_values.parquet"); update the write site that uses log_dir / "shap_values_train.parquet" and any other reads/writes that reference "shap_values_train.parquet" so they use "train_shap_values.parquet" instead (search for the exact string "shap_values_train.parquet" and replace with "train_shap_values.parquet"), ensuring consistency with the existing "test_shap_values.parquet" pattern.experiments/slurm_base_template.sh (1)
23-23: 💤 Low valueConsider quoting the variable expansion.
Shellcheck suggests double-quoting
${TASK}to prevent globbing and word splitting. While the current usage is safe given the expected values (e.g., "BinaryClassification"), quoting is a defensive best practice.Note that lines 21-22 and 24-27 also use unquoted variables in echo statements, so applying this change here alone would be inconsistent. Consider whether to quote all variable expansions in logging statements throughout the file.
🛡️ Proposed fix to add defensive quoting
-echo "Task type:" ${TASK} +echo "Task type:" "${TASK}"🤖 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 `@experiments/slurm_base_template.sh` at line 23, Quote variable expansions in logging echo statements to prevent globbing and word-splitting: change echo "Task type:" ${TASK} to use "${TASK}" and apply the same defensive quoting to the other echo usages that expand variables (the nearby echo statements at the same block that reference TASK, and the other unquoted expansions on lines around it). Locate the echo calls in this script (references to TASK and other env vars) and wrap their variable expansions in double quotes consistently.
🤖 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 `@experiments/slurm_base_template.sh`:
- Line 23: Quote variable expansions in logging echo statements to prevent
globbing and word-splitting: change echo "Task type:" ${TASK} to use "${TASK}"
and apply the same defensive quoting to the other echo usages that expand
variables (the nearby echo statements at the same block that reference TASK, and
the other unquoted expansions on lines around it). Locate the echo calls in this
script (references to TASK and other env vars) and wrap their variable
expansions in double quotes consistently.
In `@icu_benchmarks/models/train.py`:
- Line 263: Rename the train SHAP output filename to match the split-prefix
convention used by tests (change "shap_values_train.parquet" to
"train_shap_values.parquet"); update the write site that uses log_dir /
"shap_values_train.parquet" and any other reads/writes that reference
"shap_values_train.parquet" so they use "train_shap_values.parquet" instead
(search for the exact string "shap_values_train.parquet" and replace with
"train_shap_values.parquet"), ensuring consistency with the existing
"test_shap_values.parquet" pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d40bc120-0a2c-4288-a774-1343cc6ff757
📒 Files selected for processing (7)
.github/workflows/code-quality.yml.github/workflows/e2e-integration.yml.github/workflows/python-build.ymlexperiments/slurm_base_template.shicu_benchmarks/imputation/amputations.pyicu_benchmarks/models/train.pyrequirements.txt
💤 Files with no reviewable changes (2)
- requirements.txt
- icu_benchmarks/imputation/amputations.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@icu_benchmarks/imputation/diffwave.py`:
- Around line 387-395: The forward loop over residual layers was corrupted:
remove the nested/repeated loops that reuse variable `n`, stop resetting `h` to
`noise` each iteration, and ensure skip connections are accumulated exactly once
per residual block; iterate once for i in range(self.num_res_layers) (or
similar), on each iteration call self.residual_blocks[i]((h, conditional,
diffusion_step_embed)) to get (h, skip_n), set h to the returned h (not noise),
and do skip += skip_n only once per block before returning skip * math.sqrt(1.0
/ self.num_res_layers) to restore the standard DiffWave residual chain behavior
using the existing attributes `num_res_layers`, `residual_blocks`, `skip`, `h`,
`noise`, `diffusion_step_embed`, and `conditional`.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f92b23d1-126e-4512-abfe-67a4cf0f1e43
📒 Files selected for processing (3)
README.mdicu_benchmarks/imputation/diffwave.pyicu_benchmarks/models/train.py
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- icu_benchmarks/models/train.py
1 participant
Summary by CodeRabbit
Bug Fixes
Chores
Documentation
Refactor