Add BAM-MP-core model submission#381
Conversation
|
Warning Review limit reached
Next review available in: 31 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughBAM-MP-core metadata is added, and a new BAM calculator integrates checkpoint loading, periodic-graph stress defaults, and ChangesBAM-MP-core integration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CALCULATORS
participant download_checkpoint
participant BAMCalculator
participant batch_graphs
CALCULATORS->>download_checkpoint: download .pkl checkpoint when absent
download_checkpoint-->>CALCULATORS: return checkpoint path
CALCULATORS->>BAMCalculator: construct calculator
BAMCalculator->>batch_graphs: add default periodic stress
BAMCalculator->>BAMCalculator: copy energy to free_energy
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
2752318 to
5642a86
Compare
|
Pushed a cleanup update based on current Fixes included:
Local validation after the update:
Could a maintainer please apply |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
matbench_discovery/data.py (1)
472-477: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAvoid misleading error message when
id_colis missing.If
df_predsis missingid_col(e.g.,material_id),set_index(id_col)will raise aKeyError, but theexceptblock will erroneously report that the energy column was not found.Consider explicitly checking for both columns to avoid developer confusion.
💡 Proposed fix
- try: - df_out[model.label] = df_preds.set_index(id_col)[energy_col] - except KeyError as exc: - raise ValueError( - f"e_form_per_atom column not found in {pred_path}" - ) from exc + if id_col not in df_preds: + raise ValueError(f"'{id_col}' column not found in {pred_path}") + if energy_col not in df_preds: + raise ValueError(f"'{energy_col}' column not found in {pred_path}") + df_out[model.label] = df_preds.set_index(id_col)[energy_col]🤖 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 `@matbench_discovery/data.py` around lines 472 - 477, Update the assignment around df_out[model.label] to validate id_col and energy_col in df_preds separately before calling set_index. Raise a clear ValueError identifying the missing identifier column when id_col is absent, and retain the energy-column-specific error for a missing energy_col.
🧹 Nitpick comments (1)
matbench_discovery/data.py (1)
468-471: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer using the centralized constant for the canonical prediction column.
The value
"e_form_per_atom"is defined asDISCOVERY_PRED_COLinmatbench_discovery.discovery. Consider importing and using that constant instead of a hardcoded string to ensure consistency across the codebase.🤖 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 `@matbench_discovery/data.py` around lines 468 - 471, Replace the hardcoded "e_form_per_atom" assignment in the prediction-column handling with the centralized DISCOVERY_PRED_COL constant imported from matbench_discovery.discovery. Keep the existing model-specific rename logic and column checks unchanged.
🤖 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 `@matbench_discovery/data.py`:
- Around line 502-508: The cached load_discovery_predictions function
incorrectly keys only on an empty argument set and exposes mutable cached
DataFrames. Pass the selected models as an explicit hashable tuple, use that
tuple for load_df_wbm_with_preds instead of reading complete_models() inside the
cached function, and return copies of the resulting DataFrames so caller
mutations cannot alter cached state.
---
Outside diff comments:
In `@matbench_discovery/data.py`:
- Around line 472-477: Update the assignment around df_out[model.label] to
validate id_col and energy_col in df_preds separately before calling set_index.
Raise a clear ValueError identifying the missing identifier column when id_col
is absent, and retain the energy-column-specific error for a missing energy_col.
---
Nitpick comments:
In `@matbench_discovery/data.py`:
- Around line 468-471: Replace the hardcoded "e_form_per_atom" assignment in the
prediction-column handling with the centralized DISCOVERY_PRED_COL constant
imported from matbench_discovery.discovery. Keep the existing model-specific
rename logic and column checks unchanged.
🪄 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: 4f7fc588-3c1b-42b0-af36-0197bac14a66
📒 Files selected for processing (5)
.pre-commit-config.yamlmatbench_discovery/calculators.pymatbench_discovery/data.pymatbench_discovery/enums.pymodels/bam/bam-mp-core.yml
🚧 Files skipped from review as they are similar to previous changes (4)
- matbench_discovery/enums.py
- models/bam/bam-mp-core.yml
- .pre-commit-config.yaml
- matbench_discovery/calculators.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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
matbench_discovery/data.py (1)
472-477: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAvoid misleading error message when
id_colis missing.If
df_predsis missingid_col(e.g.,material_id),set_index(id_col)will raise aKeyError, but theexceptblock will erroneously report that the energy column was not found.Consider explicitly checking for both columns to avoid developer confusion.
💡 Proposed fix
- try: - df_out[model.label] = df_preds.set_index(id_col)[energy_col] - except KeyError as exc: - raise ValueError( - f"e_form_per_atom column not found in {pred_path}" - ) from exc + if id_col not in df_preds: + raise ValueError(f"'{id_col}' column not found in {pred_path}") + if energy_col not in df_preds: + raise ValueError(f"'{energy_col}' column not found in {pred_path}") + df_out[model.label] = df_preds.set_index(id_col)[energy_col]🤖 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 `@matbench_discovery/data.py` around lines 472 - 477, Update the assignment around df_out[model.label] to validate id_col and energy_col in df_preds separately before calling set_index. Raise a clear ValueError identifying the missing identifier column when id_col is absent, and retain the energy-column-specific error for a missing energy_col.
🧹 Nitpick comments (1)
matbench_discovery/data.py (1)
468-471: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer using the centralized constant for the canonical prediction column.
The value
"e_form_per_atom"is defined asDISCOVERY_PRED_COLinmatbench_discovery.discovery. Consider importing and using that constant instead of a hardcoded string to ensure consistency across the codebase.🤖 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 `@matbench_discovery/data.py` around lines 468 - 471, Replace the hardcoded "e_form_per_atom" assignment in the prediction-column handling with the centralized DISCOVERY_PRED_COL constant imported from matbench_discovery.discovery. Keep the existing model-specific rename logic and column checks unchanged.
🤖 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 `@matbench_discovery/data.py`:
- Around line 502-508: The cached load_discovery_predictions function
incorrectly keys only on an empty argument set and exposes mutable cached
DataFrames. Pass the selected models as an explicit hashable tuple, use that
tuple for load_df_wbm_with_preds instead of reading complete_models() inside the
cached function, and return copies of the resulting DataFrames so caller
mutations cannot alter cached state.
---
Outside diff comments:
In `@matbench_discovery/data.py`:
- Around line 472-477: Update the assignment around df_out[model.label] to
validate id_col and energy_col in df_preds separately before calling set_index.
Raise a clear ValueError identifying the missing identifier column when id_col
is absent, and retain the energy-column-specific error for a missing energy_col.
---
Nitpick comments:
In `@matbench_discovery/data.py`:
- Around line 468-471: Replace the hardcoded "e_form_per_atom" assignment in the
prediction-column handling with the centralized DISCOVERY_PRED_COL constant
imported from matbench_discovery.discovery. Keep the existing model-specific
rename logic and column checks unchanged.
🪄 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: 4f7fc588-3c1b-42b0-af36-0197bac14a66
📒 Files selected for processing (5)
.pre-commit-config.yamlmatbench_discovery/calculators.pymatbench_discovery/data.pymatbench_discovery/enums.pymodels/bam/bam-mp-core.yml
🚧 Files skipped from review as they are similar to previous changes (4)
- matbench_discovery/enums.py
- models/bam/bam-mp-core.yml
- .pre-commit-config.yaml
- matbench_discovery/calculators.py
🛑 Comments failed to post (1)
matbench_discovery/data.py (1)
502-508: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Risk of stale cache and mutable state leakage.
Using
@cacheon a function that depends on global state (complete_models()) and returns mutablepd.DataFrameinstances introduces two major risks:
- Stale Data: The cache key is always empty
(). Ifcli_argschanges (e.g., across different tests mocking different models), this function will continue to return stale data from its first invocation.- State Leakage: Pandas DataFrames are mutable. If any caller modifies the returned DataFrames in place, the cached state is corrupted for all subsequent callers.
Consider accepting a hashable tuple of models as an explicit argument so the cache correctly tracks inputs, and either returning copies of the DataFrames (
.copy()) or removing@cacheentirely.🤖 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 `@matbench_discovery/data.py` around lines 502 - 508, The cached load_discovery_predictions function incorrectly keys only on an empty argument set and exposes mutable cached DataFrames. Pass the selected models as an explicit hashable tuple, use that tuple for load_df_wbm_with_preds instead of reading complete_models() inside the cached function, and return copies of the resulting DataFrames so caller mutations cannot alter cached state.
5642a86 to
129738d
Compare
|
Quick ping on this PR: the branch is still current with main (0 behind, 1 ahead), and the remaining failures are the expected payload/ingest gates for the newly added bam-mp-core model. Could a maintainer apply the ingest-model label (and payloads-unchanged if appropriate) so CI can complete? |
aeb941b to
e00d7b1
Compare
e00d7b1 to
217c89a
Compare
|
Updated the PR to match the current ingestion rules: removed the manual enum/data-loader changes, kept only the BAM calculator integration plus model YAML, and switched discovery/geo-opt URLs to canonical artifacts with e_form_per_atom and structure columns. CI now has prek/scripts/vitest passing; the remaining model-pr-guard/pytest failures are the expected pre-ingestion registry/parity failures until a maintainer applies the ingest-model label. |
|
Thanks for the submission! Currently hiking the alps but either me or @CompRhys will review and merge next week. Don't worry about reopening the PR, we'll handle merge conflicts |
2 participants
Description
This PR adds the BAM-MP-core model submission.
Validation performed locally:
tests/models/: 200 passedprek run --files .pre-commit-config.yaml matbench_discovery/calculators.py matbench_discovery/enums.py models/bam/bam-mp-core.yml: passedmodels never ingested: ['bam-mp-core']Could a maintainer please apply the
ingest-modellabel forbam-mp-core? The branch is based on currentmainand keeps the resubmission to one clean commit.Checklist
Please open PRs as
draftand only mark asready to reviewafter checking off the following items:models/<arch_name>/<model_variant>.ymlfor my submission.arch_nameis the name of the architecture andmodel_variant.ymlincludes things like author details, training set names and important hyperparameters.<yyyy-mm-dd>-<model_variant>-preds.csv.gz).<yyyy-mm-dd>-wbm-IS2RE-FIRE.jsonl.gz).<yyyy-mm-dd>-phonondb-kappa-103.json.gz), with any force sets in a separate artifact.<yyyy-mm-dd>-<model_name>-md-metrics.csv.gz) — not included in this submission.<yyyy-mm-dd>-diatomics.json.gz) — not included in this submission.models/<arch_name>/<model_variant>.yml). If not using Figshare I have included the urls to the cloud storage service in the description of the PR.uv run --with-editable . scripts/ingest_model.py <model_variant>as described in the contributing guide to check metadata and generate the plots needed for submission. Maintainer ingestion viaingest-modellabel is requested above.prek installoruvx prek) and ensured all checks are passing.Marking your PR as
ready to reviewwill trigger an automated code review, please address any issues raised by the automated review. For the maintainers minimizing the final human review process enables us to merge your submissions much faster!Additional Information (Optional)
train_<arch_name>.py) if I trained a model specifically for this benchmark.readme.mdwith additional details about my model.Summary by CodeRabbit