Add eqV2_tracegrad Model#273
Conversation
for more information, see https://pre-commit.ci
|
thanks a lot for the kind words and for submitting in the meantime could you fix the lint issues and make the scripts more easily runnable by others by removing hard-coded paths like |
📝 WalkthroughWalkthroughAdds an eqV2 TraceGrad model descriptor and multiple new scripts for aggregating predictions, exporting geo‑optimized structures, WBM relaxations, and phonon/kappa evaluation. Introduces Typer CLIs, Submitit parallelism, ASE relaxations using FAIRChem OCPCalculator, per-job result aggregation, and shell runners. (50 words) Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as User/CLI
participant RJ as RelaxJob
participant ASE as ASE Optimizer
participant FC as FAIRChem OCPCalculator
participant WBM as WBM Dataset
participant Out as Output Store
CLI->>RJ: run_relax(params)
RJ->>WBM: Load dataset / split subset
RJ->>FC: Init calculator (checkpoint)
loop per structure
RJ->>ASE: Assign calculator, apply cell filter
ASE-->>RJ: Relaxed atoms + energy
RJ->>Out: Append energy + structure
end
RJ->>Out: Write gzipped JSON, wandb artifact
sequenceDiagram
autonumber
participant CLI as User/CLI
participant KR as KappaSRMERunner
participant ASE as ASE Relax (FIRE)
participant FC as FAIRChem OCPCalculator
participant P3 as Phono3py (LT Conductivity)
participant Out as Output Store
CLI->>KR: run_kappa(params, num_jobs)
KR->>FC: Init calculator
loop per structure (job shard)
KR->>ASE: Relax with symmetry constraints
ASE-->>KR: Relaxed structure
KR->>P3: Build phonon (FC2/FC3)
P3-->>KR: Frequencies, kappa
KR->>Out: Save per-structure results
end
KR->>Out: Merge job outputs, write json.gz + metrics
sequenceDiagram
autonumber
participant CLI as User/CLI
participant JP as join_predictions
participant Files as Per-model files
participant Base as Baseline eqV2 data
participant WBM as WBM CSEs
participant PMG as Pymatgen
participant Out as Output
CLI->>JP: join_predictions(input_path, out_path, opts)
JP->>Files: Read model *.json.gz / *.csv.gz
JP->>Base: Load baseline energies
JP->>WBM: Load CSE entries
JP->>PMG: Hydrate CSEs with ML energies/structures
JP->>PMG: Compute E_form/atom (MP2020 corrections opt.)
JP->>Out: Write csv.gz, -bad.csv, json.gz
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas to pay extra attention:
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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.
Actionable comments posted: 20
🧹 Nitpick comments (22)
models/eqV2_tracegrad/save_geo_opt_file.py (1)
7-21: Type annotations and minor robustness for format detection.Add type hints and skip leading whitespace before checking the first non-space char. Also handle empty files gracefully.
-def detect_file_format(file_path): - """自动检测JSON文件的格式类型""" - with gzip.open(file_path, "rt") as f: - first_char = f.read(1) +def detect_file_format(file_path: str) -> str: + """自动检测JSON文件的格式类型""" + with gzip.open(file_path, "rt") as f: + # skip leading whitespace/newlines + first_char = "" + while True: + ch = f.read(1) + if not ch or not ch.isspace(): + first_char = ch + break # JSON数组格式以 [ 开头 if first_char == "[": return "array" # 行分隔JSON通常以 { 开头 try: f.seek(0) line = f.readline().strip() json.loads(line) # 验证是否为完整JSON对象 return "lines" except json.JSONDecodeError: return "unknown"models/eqV2_tracegrad/eqV2-s-dens-tracegrad-mp.yml (1)
76-77: Fill or drop empty URLs to avoid dangling metadata.These keys are present but empty. Either provide the URLs or remove the keys to prevent downstream tooling from assuming availability.
- pred_file_url: + pred_file_url: # add URL or remove this line @@ - analysis_file_url: + analysis_file_url: # add URL or remove this line @@ - analysis_file_url: + analysis_file_url: # add URL or remove this lineAlso applies to: 88-89, 96-97
models/eqV2_tracegrad/join_fairchem_preds.py (1)
88-96: Avoid mutating private attributes; construct a new CSE instead.Directly setting
cse._energyandcse._structurerelies on private internals. Prefer creating a newComputedStructureEntry.- # cse._energy is the uncorrected energy - cse._energy = pred_energy # noqa: SLF001 - # cse._energy = pred_energy_ori # added by fywang - cse._structure = mlip_struct # noqa: SLF001 - df_fairchem.loc[mat_id, computed_structure_entry_key] = cse + df_fairchem.loc[mat_id, computed_structure_entry_key] = ComputedStructureEntry( + structure=mlip_struct, + energy=pred_energy, + parameters=cse.parameters, + data=cse.data, + )models/eqV2_tracegrad/join_fairchem_preds.sh (1)
1-1: Fix shebang (no leading space).Shellcheck SC1114: remove leading space for the shebang line.
- #!/bin/bash +#!/bin/bashmodels/eqV2_tracegrad/test_fairchem_kappa_debug.py (3)
326-336: Typer help strings contain full-width punctuation; switch to ASCII for linting/consistency.Replace Chinese full-width punctuation with ASCII to satisfy linters (RUF001) and keep CLI help consistent.
- checkpoint_path: Annotated[Path, typer.Option(help="路径，包含 checkpoint.pt")], + checkpoint_path: Annotated[Path, typer.Option(help="路径, 包含 checkpoint.pt")], @@ - model_name: Annotated[str, typer.Option(help="输出目录的子目录名")], - identifier: Annotated[str, typer.Option(help="运行标识，用于文件夹命名")], - atom_disp: Annotated[float, typer.Option(help="用于计算声子性质的位移距离")] = 0.03, - seed: Annotated[int, typer.Option(help="随机种子")] = 42, - num_jobs: Annotated[int, typer.Option(help="并行任务数")] = 10, - debug: Annotated[bool, typer.Option(help="调试模式，仅运行一个 job")] = False, - slurm_timeout: Annotated[int, typer.Option(help="slurm 超时时间（小时）")] = 80, + model_name: Annotated[str, typer.Option(help="输出目录的子目录名")], + identifier: Annotated[str, typer.Option(help="运行标识, 用于文件夹命名")], + atom_disp: Annotated[float, typer.Option(help="用于计算声子性质的位移距离")] = 0.03, + seed: Annotated[int, typer.Option(help="随机种子")] = 42, + num_jobs: Annotated[int, typer.Option(help="并行任务数")] = 10, + debug: Annotated[bool, typer.Option(help="调试模式, 仅运行一个 job")] = False, + slurm_timeout: Annotated[int, typer.Option(help="slurm 超时时间(小时)")] = 80,
341-349: Executor is configured but not used; unused args (num_jobs/debug).For a “debug” script, either remove the submitit executor and the unused args, or wire them up so users can run N jobs. Presently they’re dead code.
Two options:
- Keep it purely local: drop executor config and arguments.
- Or implement simple job-splitting using
num_jobsand loop overrange(num_jobs)submittingjob.run(job_number=i)to the executor.I can provide a minimal executor-backed variant if you want multi-job runs here.
Also applies to: 351-361
162-170: Relaxation step-check and stress reporting.
- FIRE exposes
optimizer.nsteps; the change is fine. Consider logging both steps and whetherreached_max_steps.atoms.get_stress()returns 6 components (Voigt). Reshaping to (2, 3) and takingmax(axis=1)isn’t meaningful. Either keep the 6-vector or computenp.max(np.abs(stress)).- max_stress = ( - atoms.get_stress().reshape((2, 3), order="C").max(axis=1) - ) + stress_voigt = atoms.get_stress() + max_stress = float(abs(stress_voigt).max())Also applies to: 171-176
models/eqV2_tracegrad/test_fairchem_kappa.py (7)
70-74: Type annotation for env_vars is incorrect (breaks IDEs/linters).
env_varsis annotated asNone, but the code treats it like a mapping. Use a proper optional mapping type.- def run(self, env_vars: None, job_number: int = 0) -> None: + def run(self, env_vars: dict[str, str] | None, job_number: int = 0) -> None:
110-130: Potential concurrent write race to run_params.json.All jobs write the same
run_params.jsonin a sharedout_dir, causing non-deterministic content or partial writes. Make this per-job.- with open(save_dir / "run_params.json", mode="w") as file: + with open(save_dir / f"run_params_{job_number:03}.json", mode="w") as file: json.dump(run_params, file, indent=4)
175-177: Incorrect stress reshaping; use norm/abs-based scalar instead.Reshaping a 6-component Voigt stress vector into shape (2, 3) and taking
max(axis=1)is arbitrary and potentially misleading. Use an absolute max over the Voigt vector or compute a proper norm.- max_stress = ( - atoms.get_stress().reshape((2, 3), order="C").max(axis=1) - ) + # stress returned as Voigt vector [sxx, syy, szz, syz, sxz, sxy] + stress = atoms.get_stress() + max_stress = float(abs(stress).max())
301-319: Combine step may run on multiple workers; harmless but noisy.Each job may enter the combine block simultaneously when all shards exist, duplicating the combine work and console logs. Consider gating combine to a single job (e.g.,
job_number == 0) to cut noise.- if len(file_list) == self.num_jobs: + if job_number == 0 and len(file_list) == self.num_jobs:
327-336: Fix ambiguous full-width punctuation in Typer help strings (ruff RUF001).Use ASCII punctuation to satisfy linters.
- checkpoint_path: Annotated[Path, typer.Option(help="路径，包含 checkpoint.pt")], + checkpoint_path: Annotated[Path, typer.Option(help="Path containing checkpoint.pt")], @@ - model_name: Annotated[str, typer.Option(help="输出目录的子目录名")], - identifier: Annotated[str, typer.Option(help="运行标识，用于文件夹命名")], - atom_disp: Annotated[float, typer.Option(help="用于计算声子性质的位移距离")] = 0.03, - seed: Annotated[int, typer.Option(help="随机种子")] = 42, - num_jobs: Annotated[int, typer.Option(help="并行任务数")] = 10, - debug: Annotated[bool, typer.Option(help="调试模式，仅运行一个 job")] = False, - slurm_timeout: Annotated[int, typer.Option(help="slurm 超时时间（小时）")] = 80, + model_name: Annotated[str, typer.Option(help="Subdirectory name under out-path")], + identifier: Annotated[str, typer.Option(help="Run identifier used for folder naming")], + atom_disp: Annotated[float, typer.Option(help="Displacement distance for phonon calc")] = 0.03, + seed: Annotated[int, typer.Option(help="Random seed")] = 42, + num_jobs: Annotated[int, typer.Option(help="Number of parallel jobs")] = 10, + debug: Annotated[bool, typer.Option(help="Debug mode: run a single job")] = False, + slurm_timeout: Annotated[int, typer.Option(help="Slurm time out in hours")] = 80,
354-367: Comment contradicts code regarding num_jobs in debug mode.The comment says “设置为0处理前3个样本” but you pass
num_jobs=1. Either set it to 0 or update the comment to avoid confusion.- num_jobs=1, # 设置为0处理前3个样本 + num_jobs=1, # debug: a single job
372-391: Standardize GPU assignment using Submitit’sgpus_per_taskinstead of manualCUDA_VISIBLE_DEVICESManual setting of
CUDA_VISIBLE_DEVICESacross test suites will break or collide whennum_jobsexceeds the physical GPU count. Since you’re already calling:executor.update_parameters( cpus_per_task=1, gpus_per_task=1, … )you can remove all
env_vars = {"CUDA_VISIBLE_DEVICES": …}assignments and let Submitit handle GPU placement. If you still need explicit indexing, guard against the available GPUs (e.g. modulotorch.cuda.device_count()).Files/locations to refactor:
- models/eqV2_tracegrad/test_fairchem_kappa.py
• debug mode: lines 357–359
• batch loop: lines 375–379- models/eqV2_tracegrad/test_fairchem_tracegrad_discovery.py
• debug mode: lines 268–269
• batch loop: lines 273–277- models/hienet/test_hienet_kappa.py
• lines 64–66- models/hienet/test_hienet_discovery.py
• lines 65–67By removing these overrides, you ensure robust GPU scheduling even when
num_jobs > available GPUs.models/eqV2_tracegrad/test_fairchem_kappa.sh (1)
3-10: Avoid hard-coded absolute paths; make script relocatable.Consider using environment variables (e.g., FAIRCHEM_CHECKPOINT, MBD_OUT) or accepting these as positional args to the script.
models/eqV2_tracegrad/test_fairchem_tracegrad.sh (3)
1-1: Remove leading space before shebang (SC1114).- #!/bin/bash +#!/usr/bin/env bash +set -euo pipefail
16-25: Same trailing backslash in commented block (SC1143).Remove the trailing backslash to avoid confusion, or move the comment above.
-# --num-jobs 1\ +# --num-jobs 1
5-14: Parameterize absolute paths for portability.Use env vars (e.g., OUT_PATH, CKPT) or accept inputs as args. This aligns with maintainers’ request and CI runners.
models/eqV2_tracegrad/test_fairchem_tracegrad_discovery.py (4)
54-59: Add return type annotation for private helper (ruff ANN202).- def _add_fairchem_to_sys_path(self): + def _add_fairchem_to_sys_path(self) -> None:
74-79: Type annotation for env_vars is incorrect.
env_varsshould be optional mapping.- env_vars: None, + env_vars: dict[str, str] | None,
90-96: Use current device index for device name.You print the current device id but then query
get_device_name(0), which may not correspond to the current device when CUDA_VISIBLE_DEVICES is set.- print(f"Current CUDA device: {torch.cuda.current_device()}") - print(f"Device name: {torch.cuda.get_device_name(0)}") + dev = torch.cuda.current_device() + print(f"Current CUDA device: {dev}") + print(f"Device name: {torch.cuda.get_device_name(dev)}")
268-277: GPU assignment via CUDA_VISIBLE_DEVICES equals job index can over-subscribe VRAM.Prefer letting Slurm/Submitit handle placement when using
gpus_per_task=1. If you must set it, guard againstjob_number >= num_visible_gpus.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
models/eqV2_tracegrad/eqV2-s-dens-tracegrad-mp.yml(1 hunks)models/eqV2_tracegrad/join_fairchem_preds.py(1 hunks)models/eqV2_tracegrad/join_fairchem_preds.sh(1 hunks)models/eqV2_tracegrad/save_geo_opt_file.py(1 hunks)models/eqV2_tracegrad/test_fairchem_kappa.py(1 hunks)models/eqV2_tracegrad/test_fairchem_kappa.sh(1 hunks)models/eqV2_tracegrad/test_fairchem_kappa_debug.py(1 hunks)models/eqV2_tracegrad/test_fairchem_tracegrad.sh(1 hunks)models/eqV2_tracegrad/test_fairchem_tracegrad_discovery.py(1 hunks)models/eqV2_tracegrad/test_fairchem_tracegrad_discovery_debug.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (6)
models/eqV2_tracegrad/join_fairchem_preds.py (3)
matbench_discovery/enums.py (3)
DataFiles(472-573)MbdKey(46-123)path(549-573)matbench_discovery/data.py (1)
as_dict_handler(42-51)matbench_discovery/energy.py (1)
get_e_form_per_atom(149-182)
models/eqV2_tracegrad/test_fairchem_kappa_debug.py (4)
matbench_discovery/enums.py (4)
metadata(368-377)DataFiles(472-573)path(549-573)base_dir(209-211)matbench_discovery/phonons/__init__.py (1)
check_imaginary_freqs(16-42)models/eqV2_tracegrad/test_fairchem_kappa.py (4)
seed_everywhere(45-48)KappaSRMERunner(51-323)run(70-323)run_kappa(326-391)matbench_discovery/phonons/thermal_conductivity.py (4)
init_phono3py(110-152)get_fc2_and_freqs(155-188)calculate_fc3_set(68-107)calculate_conductivity(212-274)
models/eqV2_tracegrad/test_fairchem_kappa.py (3)
matbench_discovery/enums.py (4)
metadata(368-377)DataFiles(472-573)path(549-573)base_dir(209-211)matbench_discovery/phonons/__init__.py (1)
check_imaginary_freqs(16-42)matbench_discovery/phonons/thermal_conductivity.py (4)
init_phono3py(110-152)get_fc2_and_freqs(155-188)calculate_fc3_set(68-107)calculate_conductivity(212-274)
models/eqV2_tracegrad/save_geo_opt_file.py (1)
matbench_discovery/enums.py (1)
geo_opt_path(418-436)
models/eqV2_tracegrad/test_fairchem_tracegrad_discovery_debug.py (5)
matbench_discovery/enums.py (3)
path(549-573)MbdKey(46-123)Task(127-141)matbench_discovery/data.py (1)
as_dict_handler(42-51)matbench_discovery/plots.py (1)
wandb_scatter(696-722)models/eqV2_tracegrad/test_fairchem_kappa_debug.py (1)
run(68-323)models/eqV2_tracegrad/test_fairchem_kappa.py (1)
run(70-323)
models/eqV2_tracegrad/test_fairchem_tracegrad_discovery.py (3)
matbench_discovery/enums.py (2)
MbdKey(46-123)Task(127-141)matbench_discovery/data.py (1)
as_dict_handler(42-51)matbench_discovery/plots.py (1)
wandb_scatter(696-722)
🪛 Shellcheck (0.10.0)
models/eqV2_tracegrad/test_fairchem_kappa.sh
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
[error] 11-11: This backslash is part of a comment and does not continue the line.
(SC1143)
models/eqV2_tracegrad/test_fairchem_tracegrad.sh
[error] 1-1: Remove leading spaces before the shebang.
(SC1114)
models/eqV2_tracegrad/join_fairchem_preds.sh
[error] 1-1: Remove leading spaces before the shebang.
(SC1114)
🪛 Ruff (0.12.2)
models/eqV2_tracegrad/join_fairchem_preds.py
66-66: Comment contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF003)
models/eqV2_tracegrad/test_fairchem_kappa_debug.py
327-327: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
330-330: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
333-333: Unused function argument: num_jobs
(ARG001)
334-334: Boolean default positional argument in function definition
(FBT002)
334-334: Unused function argument: debug
(ARG001)
334-334: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
335-335: String contains ambiguous （ (FULLWIDTH LEFT PARENTHESIS). Did you mean ( (LEFT PARENTHESIS)?
(RUF001)
335-335: String contains ambiguous ） (FULLWIDTH RIGHT PARENTHESIS). Did you mean ) (RIGHT PARENTHESIS)?
(RUF001)
models/eqV2_tracegrad/test_fairchem_kappa.py
327-327: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
330-330: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
334-334: Boolean default positional argument in function definition
(FBT002)
334-334: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
335-335: String contains ambiguous （ (FULLWIDTH LEFT PARENTHESIS). Did you mean ( (LEFT PARENTHESIS)?
(RUF001)
335-335: String contains ambiguous ） (FULLWIDTH RIGHT PARENTHESIS). Did you mean ) (RIGHT PARENTHESIS)?
(RUF001)
356-356: Comment contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF003)
356-356: Comment contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF003)
372-372: Comment contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF003)
models/eqV2_tracegrad/save_geo_opt_file.py
7-7: Missing return type annotation for public function detect_file_format
Add return type annotation: str | None
(ANN201)
7-7: Missing type annotation for function argument file_path
(ANN001)
19-19: Consider moving this statement to an else block
(TRY300)
25-25: Line too long (120 > 88)
(E501)
26-26: Line too long (117 > 88)
(E501)
models/eqV2_tracegrad/test_fairchem_tracegrad_discovery_debug.py
40-40: Line too long (147 > 88)
(E501)
41-41: Line too long (129 > 88)
(E501)
80-80: info() call on root logger
(LOG015)
165-165: info() call on root logger
(LOG015)
177-177: Line too long (97 > 88)
(E501)
178-178: Line too long (112 > 88)
(E501)
191-191: exception() call on root logger
(LOG015)
models/eqV2_tracegrad/test_fairchem_tracegrad_discovery.py
54-54: Missing return type annotation for private function _add_fairchem_to_sys_path
Add return type annotation: None
(ANN202)
90-90: Comment contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF003)
100-100: info() call on root logger
(LOG015)
176-176: info() call on root logger
(LOG015)
197-197: exception() call on root logger
(LOG015)
🪛 Gitleaks (8.27.2)
models/eqV2_tracegrad/eqV2-s-dens-tracegrad-mp.yml
2-2: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (5)
models/eqV2_tracegrad/eqV2-s-dens-tracegrad-mp.yml (1)
99-119: Verify metrics consistency (FPR/TNR vs Accuracy) and units.
- most_stable_10k: TPR=1.0 and FPR=1.0 implies predicting all positives; check if Accuracy (0.976) aligns with TP/FP counts given class distribution.
- missing_percent is annotated as “fraction” but values include a percent sign (e.g., "0.00%"). Use a numeric fraction or correct the comment.
If needed, I can help recompute and update these blocks from your result files.
Also applies to: 120-139, 140-158
models/eqV2_tracegrad/join_fairchem_preds.py (1)
40-46: Minor: glob logic and logging are fine.Pattern and discovery are correct given the naming scheme.
models/eqV2_tracegrad/test_fairchem_kappa_debug.py (1)
217-236: FC3 gating logic reads well; continue if fail.Conditioning FC3 on non-imaginary freqs and symmetry is a reasonable gate. The warnings and skip behavior are appropriate.
Also applies to: 240-249
models/eqV2_tracegrad/test_fairchem_tracegrad_discovery_debug.py (2)
80-91: Root logger usage is fine for a debug script.Given this is a debug/local runner, using the root logger and suppressing detailed setup is acceptable.
195-222: CLI defaults mirror production; good parity.The Typer interface is consistent with the production runner, which keeps UX aligned for users switching between modes.
| df_fairchem_ori = pd.read_csv( | ||
| "/data/fywang/code/matbench-discovery/pred_results/eqV2-small-dens/test/eqV2-s-dens-mp-ori.csv.gz", | ||
| compression="gzip", | ||
| ).set_index(Key.mat_id) # 开源结果 | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove hard-coded baseline path; make it optional CLI input.
Hard-coded absolute paths break reproducibility. Make the “eqV2 open-data baseline” path a parameter.
def join_predictions(
@@
apply_mp_corrections: Annotated[
bool, typer.Option(help="Apply MP 2020 corrections to formation energies")
] = False,
+ baseline_path: Annotated[
+ str | None,
+ typer.Option(help="Optional path to baseline eqV2-ori energies (csv.gz)"),
+ ] = None,
) -> None:
@@
- df_fairchem_ori = pd.read_csv(
- "/data/fywang/code/matbench-discovery/pred_results/eqV2-small-dens/test/eqV2-s-dens-mp-ori.csv.gz",
- compression="gzip",
- ).set_index(Key.mat_id) # 开源结果
-
- # 合并df_fairchem_ori中的pred_energy到df_fairchem，根据mat_id匹配
- df_fairchem = df_fairchem.merge(
- df_fairchem_ori[["eqV2-31M-dens-MP-p5_energy"]],
- left_index=True,
- right_index=True,
- )
+ if baseline_path:
+ df_fairchem_ori = pd.read_csv(baseline_path, compression="gzip").set_index(
+ Key.mat_id
+ ) # 开源结果
+ # 合并df_fairchem_ori中的pred_energy到df_fairchem，根据mat_id匹配
+ df_fairchem = df_fairchem.merge(
+ df_fairchem_ori[["eqV2-31M-dens-MP-p5_energy"]],
+ left_index=True,
+ right_index=True,
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| df_fairchem_ori = pd.read_csv( | |
| "/data/fywang/code/matbench-discovery/pred_results/eqV2-small-dens/test/eqV2-s-dens-mp-ori.csv.gz", | |
| compression="gzip", | |
| ).set_index(Key.mat_id) # 开源结果 | |
| def join_predictions( | |
| @@ | |
| apply_mp_corrections: Annotated[ | |
| bool, typer.Option(help="Apply MP 2020 corrections to formation energies") | |
| ] = False, | |
| baseline_path: Annotated[ | |
| str | None, | |
| typer.Option(help="Optional path to baseline eqV2-ori energies (csv.gz)"), | |
| ] = None, | |
| ) -> None: | |
| @@ | |
| - df_fairchem_ori = pd.read_csv( | |
| - "/data/fywang/code/matbench-discovery/pred_results/eqV2-small-dens/test/eqV2-s-dens-mp-ori.csv.gz", | |
| - compression="gzip", | |
| - ).set_index(Key.mat_id) # 开源结果 | |
| - | |
| - # 合并df_fairchem_ori中的pred_energy到df_fairchem，根据mat_id匹配 | |
| - df_fairchem = df_fairchem.merge( | |
| - df_fairchem_ori[["eqV2-31M-dens-MP-p5_energy"]], | |
| - left_index=True, | |
| - right_index=True, | |
| if baseline_path: | |
| df_fairchem_ori = ( | |
| pd.read_csv(baseline_path, compression="gzip") | |
| .set_index(Key.mat_id) | |
| ) # 开源结果 | |
| # 合并 df_fairchem_ori 中的 pred_energy 到 df_fairchem，根据 mat_id 匹配 | |
| df_fairchem = df_fairchem.merge( | |
| df_fairchem_ori[["eqV2-31M-dens-MP-p5_energy"]], | |
| left_index=True, | |
| right_index=True, | |
| ) |
| if apply_mp_corrections is True: | ||
| # apply energy corrections | ||
| processed = MaterialsProject2020Compatibility().process_entries( | ||
| df_fairchem[computed_structure_entry_key], verbose=True, clean=True | ||
| ) | ||
| if len(processed) != len(df_fairchem): | ||
| raise ValueError( | ||
| f"not all entries processed: {len(processed)=} {len(df_fairchem)=}" | ||
| ) | ||
|
|
There was a problem hiding this comment.
MP2020 corrections are computed but never applied to the CSEs used below.
process_entries() returns corrected entries; you only check count. You must write them back before computing formation energies.
if apply_mp_corrections is True:
# apply energy corrections
processed = MaterialsProject2020Compatibility().process_entries(
df_fairchem[computed_structure_entry_key], verbose=True, clean=True
)
if len(processed) != len(df_fairchem):
raise ValueError(
f"not all entries processed: {len(processed)=} {len(df_fairchem)=}"
)
+ # Replace uncorrected entries with corrected ones in the same order
+ df_fairchem[computed_structure_entry_key] = processed📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if apply_mp_corrections is True: | |
| # apply energy corrections | |
| processed = MaterialsProject2020Compatibility().process_entries( | |
| df_fairchem[computed_structure_entry_key], verbose=True, clean=True | |
| ) | |
| if len(processed) != len(df_fairchem): | |
| raise ValueError( | |
| f"not all entries processed: {len(processed)=} {len(df_fairchem)=}" | |
| ) | |
| if apply_mp_corrections is True: | |
| # apply energy corrections | |
| processed = MaterialsProject2020Compatibility().process_entries( | |
| df_fairchem[computed_structure_entry_key], verbose=True, clean=True | |
| ) | |
| if len(processed) != len(df_fairchem): | |
| raise ValueError( | |
| f"not all entries processed: {len(processed)=} {len(df_fairchem)=}" | |
| ) | |
| # Replace uncorrected entries with corrected ones in the same order | |
| df_fairchem[computed_structure_entry_key] = processed |
🤖 Prompt for AI Agents
In models/eqV2_tracegrad/join_fairchem_preds.py around lines 98 to 107, you call
MaterialsProject2020Compatibility().process_entries(...) and only check that
counts match, but you never replace the CSEs in df_fairchem with the corrected
entries; write the returned processed entries back into
df_fairchem[computed_structure_entry_key] (preserving row order or mapping by
index if process_entries reorders) before any formation-energy computation, so
downstream code uses the corrected entries.
| df_fairchem.select_dtypes("number").to_csv(out_path / f"{model_name}.csv.gz") | ||
| df_bad = df_fairchem[bad_mask].drop( | ||
| columns=[computed_structure_entry_key, struct_col] | ||
| ) | ||
| df_bad[MbdKey.e_form_dft] = df_wbm[MbdKey.e_form_dft] | ||
| df_bad.to_csv(f"{out_path}-bad.csv") | ||
|
|
||
| df_fairchem.reset_index().to_json( | ||
| out_path / f"{model_name}.json.gz", default_handler=as_dict_handler | ||
| ) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ensure compression for .csv.gz and .json.gz outputs; also write -bad.csv as gzip.
Explicitly set compression to avoid uncompressed files with .gz suffix. Also avoid SettingWithCopy on df_bad, and keep consistent gz for the “bad” file.
- df_fairchem.select_dtypes("number").to_csv(out_path / f"{model_name}.csv.gz")
+ df_fairchem.select_dtypes("number").to_csv(
+ out_path / f"{model_name}.csv.gz", compression="gzip"
+ )
df_bad = df_fairchem[bad_mask].drop(
columns=[computed_structure_entry_key, struct_col]
)
- df_bad[MbdKey.e_form_dft] = df_wbm[MbdKey.e_form_dft]
- df_bad.to_csv(f"{out_path}-bad.csv")
+ df_bad = df_bad.assign(**{MbdKey.e_form_dft: df_wbm.loc[df_bad.index, MbdKey.e_form_dft]})
+ df_bad.to_csv(out_path / f"{model_name}-bad.csv.gz", compression="gzip")
@@
- df_fairchem.reset_index().to_json(
- out_path / f"{model_name}.json.gz", default_handler=as_dict_handler
- )
+ df_fairchem.reset_index().to_json(
+ out_path / f"{model_name}.json.gz",
+ compression="gzip",
+ default_handler=as_dict_handler,
+ orient="records",
+ )🌐 Web query:
Does pandas DataFrame.to_json support a compression parameter?
💡 Result:
Yes. DataFrame.to_json accepts a compression parameter (str or dict, default "infer") and will compress the output when you pass a filename. (pandas.pydata.org)
Examples:
- Let pandas infer from the extension:
df.to_json("out.json.gz") # compression inferred from .gz - Specify compression explicitly:
df.to_json("out.json.gz", compression="gzip") - Pass compression options as a dict:
df.to_json("out.json.gz", compression={"method": "gzip", "compresslevel": 1, "mtime": 1}). (pandas.pydata.org)
Note: compression is only applied when the first argument is a filename (if path_or_buf is None the method returns a JSON string; if you pass a file-like object you can wrap it yourself with gzip.open/etc.). (pandas.pydata.org)
Citations:
- 1: https://pandas.pydata.org/pandas-docs/version/1.4.1/reference/api/pandas.DataFrame.to_json.html?utm_source=chatgpt.com
- 2: https://pandas.pydata.org/pandas-docs/dev/reference/api/pandas.DataFrame.to_json.html?utm_source=chatgpt.com
- 3: https://pandas.pydata.org/pandas-docs/version/1.2.3/reference/api/pandas.DataFrame.to_json.html?utm_source=chatgpt.com
Add gzip compression to CSV/JSON outputs and fix df_bad assignment
Ensure all “.gz” files are actually compressed and avoid a SettingWithCopy on df_bad. In models/eqV2_tracegrad/join_fairchem_preds.py (lines 127–136):
- df_fairchem.select_dtypes("number").to_csv(out_path / f"{model_name}.csv.gz")
+ df_fairchem.select_dtypes("number").to_csv(
+ out_path / f"{model_name}.csv.gz",
+ compression="gzip",
+ )
df_bad = df_fairchem[bad_mask].drop(
columns=[computed_structure_entry_key, struct_col]
)
- df_bad[MbdKey.e_form_dft] = df_wbm[MbdKey.e_form_dft]
- df_bad.to_csv(f"{out_path}-bad.csv")
+ df_bad = df_bad.assign(
+ **{MbdKey.e_form_dft: df_wbm.loc[df_bad.index, MbdKey.e_form_dft]}
+ )
+ df_bad.to_csv(
+ out_path / f"{model_name}-bad.csv.gz",
+ compression="gzip",
+ )
- df_fairchem.reset_index().to_json(
- out_path / f"{model_name}.json.gz", default_handler=as_dict_handler
- )
+ df_fairchem.reset_index().to_json(
+ out_path / f"{model_name}.json.gz",
+ compression="gzip",
+ default_handler=as_dict_handler,
+ orient="records",
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| df_fairchem.select_dtypes("number").to_csv(out_path / f"{model_name}.csv.gz") | |
| df_bad = df_fairchem[bad_mask].drop( | |
| columns=[computed_structure_entry_key, struct_col] | |
| ) | |
| df_bad[MbdKey.e_form_dft] = df_wbm[MbdKey.e_form_dft] | |
| df_bad.to_csv(f"{out_path}-bad.csv") | |
| df_fairchem.reset_index().to_json( | |
| out_path / f"{model_name}.json.gz", default_handler=as_dict_handler | |
| ) | |
| df_fairchem.select_dtypes("number").to_csv( | |
| out_path / f"{model_name}.csv.gz", | |
| compression="gzip", | |
| ) | |
| df_bad = df_fairchem[bad_mask].drop( | |
| columns=[computed_structure_entry_key, struct_col] | |
| ) | |
| df_bad = df_bad.assign( | |
| **{MbdKey.e_form_dft: df_wbm.loc[df_bad.index, MbdKey.e_form_dft]} | |
| ) | |
| df_bad.to_csv( | |
| out_path / f"{model_name}-bad.csv.gz", | |
| compression="gzip", | |
| ) | |
| df_fairchem.reset_index().to_json( | |
| out_path / f"{model_name}.json.gz", | |
| compression="gzip", | |
| default_handler=as_dict_handler, | |
| orient="records", | |
| ) |
🤖 Prompt for AI Agents
In models/eqV2_tracegrad/join_fairchem_preds.py around lines 127–136, the
CSV/JSON outputs are named with “.gz” but not actually compressed and df_bad is
modified in a way that can trigger a SettingWithCopy warning; fix by making
df_bad a true copy (e.g., append .copy() after the .drop(...) call) and perform
assignment with .loc (df_bad.loc[:, MbdKey.e_form_dft] =
df_wbm[MbdKey.e_form_dft]), write the numeric CSV and the bad-rows CSV with gzip
compression (use pandas to_csv with compression='gzip' or write via gzip.open)
and write the JSON in compressed form (either pandas to_json with
compression='gzip' or write the JSON string to a gzip file) ensuring filenames
end with .csv.gz / .json.gz as intended.
| @@ -0,0 +1,15 @@ | |||
| #!/bin/bash | |||
|
|
|||
| export PYTHONPATH=/data/fywang/code/fairchem-main/src:$PYTHONPATH | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove hard-coded PYTHONPATH; rely on declared script dependencies and uv run.
Avoid machine-specific paths. Use uv run so dependencies are installed automatically per the contribution guide.
-export PYTHONPATH=/data/fywang/code/fairchem-main/src:$PYTHONPATH
+# Run with uv so dependencies declared in the script are auto-installed:
+# uv run models/eqV2_tracegrad/join_fairchem_preds.py ...📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export PYTHONPATH=/data/fywang/code/fairchem-main/src:$PYTHONPATH | |
| # Run with uv so dependencies declared in the script are auto-installed: | |
| # uv run models/eqV2_tracegrad/join_fairchem_preds.py ... |
🤖 Prompt for AI Agents
In models/eqV2_tracegrad/join_fairchem_preds.sh around line 3, remove the
hard-coded export of PYTHONPATH and instead run the Python command via the
project's task runner so dependencies are installed automatically; delete the
export line and replace direct python invocations with "uv run <script or
module>" (or wrap the script execution in uv run), and ensure the script relies
on declared dependencies in pyproject/uv configuration per the contribution
guide.
| sys.path.insert(0, "/data/fywang/code/fairchem-tracegrad-hanhai/src") | ||
| import json |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove hard-coded sys.path insertion; rely on installed packages or uv script deps.
Local path hacks break reproducibility and CI. Install the dependency or declare it in uv scripts so uv run can resolve it.
-import sys
-
-sys.path.insert(0, "/data/fywang/code/fairchem-tracegrad-hanhai/src")
+import sys📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sys.path.insert(0, "/data/fywang/code/fairchem-tracegrad-hanhai/src") | |
| import json | |
| import sys | |
| import json |
🤖 Prompt for AI Agents
In models/eqV2_tracegrad/test_fairchem_tracegrad_discovery.py around lines 3 to
4, remove the hard-coded sys.path.insert call that prepends a local source path;
instead declare the project dependency properly so the test imports resolve via
installed packages or the uv run script. Fix by removing the sys.path
modification, adding the package to test-time dependencies (e.g.,
requirements/dev or pyproject/uv script), or ensuring the test environment
installs the src package so imports work without path hacks.
| BASE_PATH = Path("/data/fywang/code/matbench-discovery/data/wbm") | ||
| DATABASE_PATH = { | ||
| Task.IS2RE: str(BASE_PATH / "WBM_IS2RE_initial_atoms.aselmdb"), | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Avoid hard-coded dataset path; make it configurable.
Hard-coding /data/.../WBM_IS2RE_initial_atoms.aselmdb ties execution to your machine. Either:
- use a DataFiles enum if available (e.g.,
DataFiles.wbm_is2re.path), or - accept a
--db-pathCLI option and default from repo data if present.
I can help wire this in if you confirm preferred source.
To locate a suitable DataFiles entry, run:
🏁 Script executed:
#!/bin/bash
# Try to find an enum or path for WBM IS2RE in the codebase
rg -n -C2 -P 'WBM_IS2RE|aselmdb|wbm.*is2re' --type=pyLength of output: 3253
🏁 Script executed:
#!/bin/bash
# Search for DataFiles enum or class to use for DB paths
rg -n -P 'class\s+DataFiles|enum\s+DataFiles|DataFiles\.' --type=pyLength of output: 12040
🏁 Script executed:
#!/bin/bash
# Show imports and context for DataFiles usage
sed -n '1,50p' models/eqV2_tracegrad/test_fairchem_tracegrad_discovery.pyLength of output: 1554
Use the shared DataFiles enum for the DB path instead of a hard-coded absolute path
Replace the hard-coded BASE_PATH/DATABASE_PATH in
models/eqV2_tracegrad/test_fairchem_tracegrad_discovery.py (lines 33–36) with the built-in DataFiles entry:
• Import and use DataFiles.wbm_initial_atoms.path for Task.IS2RE
• (Optional) Expose a --db-path CLI flag defaulting to that same value
Suggested diff:
-from pathlib import Path
-
-BASE_PATH = Path("/data/fywang/code/matbench-discovery/data/wbm")
-DATABASE_PATH = {
- Task.IS2RE: str(BASE_PATH / "WBM_IS2RE_initial_atoms.aselmdb"),
-}
+from matbench_discovery.enums import DataFiles
+
+DATABASE_PATH = {
+ Task.IS2RE: DataFiles.wbm_initial_atoms.path,
+}If you’d like a CLI override, you can add:
db_path: Path = typer.Option(
DataFiles.wbm_initial_atoms.path,
"--db-path",
help="Path to the WBM IS2RE ASELMDB file",
)
# …then use db_path instead of DataFiles.wbm_initial_atoms.path directlyLet me know if you need help wiring up the CLI option.
🤖 Prompt for AI Agents
In models/eqV2_tracegrad/test_fairchem_tracegrad_discovery.py around lines 33 to
36, replace the hard-coded BASE_PATH and DATABASE_PATH values with the shared
DataFiles enum entry: use DataFiles.wbm_initial_atoms.path for Task.IS2RE
(import DataFiles from its module), and remove the absolute Path("/data/...")
constants; optionally add a typer CLI option db_path: Path =
typer.Option(DataFiles.wbm_initial_atoms.path, "--db-path", help="Path to the
WBM IS2RE ASELMDB file") and use that variable wherever the DB path is consumed
to allow overrides.
| # 设置 submitit Executor 来管理并行任务 | ||
| executor = AutoExecutor(folder=out_path) | ||
| executor.update_parameters( | ||
| cpus_per_task=1, | ||
| slurm_gpus_per_task=1, # 每个任务使用一个 GPU | ||
| nodes=1, | ||
| timeout_min=60 * slurm_timeout, | ||
| mem_gb=80, | ||
| ) |
There was a problem hiding this comment.
Submitit param name likely invalid: use gpus_per_task (or gpus_per_node).
slurm_gpus_per_task isn’t a standard Submitit parameter and will be ignored. Use gpus_per_task=1 as in your other script, or gpus_per_node depending on your cluster policy.
- cpus_per_task=1,
- slurm_gpus_per_task=1, # 每个任务使用一个 GPU
+ cpus_per_task=1,
+ gpus_per_task=1, # one GPU per task
nodes=1,
timeout_min=60 * slurm_timeout,
mem_gb=80,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # 设置 submitit Executor 来管理并行任务 | |
| executor = AutoExecutor(folder=out_path) | |
| executor.update_parameters( | |
| cpus_per_task=1, | |
| slurm_gpus_per_task=1, # 每个任务使用一个 GPU | |
| nodes=1, | |
| timeout_min=60 * slurm_timeout, | |
| mem_gb=80, | |
| ) | |
| # 设置 submitit Executor 来管理并行任务 | |
| executor = AutoExecutor(folder=out_path) | |
| executor.update_parameters( | |
| cpus_per_task=1, | |
| gpus_per_task=1, # one GPU per task | |
| nodes=1, | |
| timeout_min=60 * slurm_timeout, | |
| mem_gb=80, | |
| ) |
🤖 Prompt for AI Agents
In models/eqV2_tracegrad/test_fairchem_tracegrad_discovery.py around lines 257
to 265, the Submitit parameter slurm_gpus_per_task is invalid and will be
ignored; replace it with the correct Submitit parameter (e.g., gpus_per_task=1
or gpus_per_node=1 depending on cluster policy) so the job actually requests GPU
resources, keep other executor.update_parameters entries unchanged, and ensure
consistency with other scripts that use gpus_per_task.
| export WANDB_MODE=offline | ||
| export PYTHONPATH=/data/fywang/code/fairchem-tracegrad-hanhai/src:$PYTHONPATH |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid hard-coded PYTHONPATH; prefer uv-managed deps.
Make this reproducible for others without your local layout.
-export WANDB_MODE=offline
-export PYTHONPATH=/data/fywang/code/fairchem-tracegrad-hanhai/src:$PYTHONPATH
+export WANDB_MODE=offline
+# Dependencies should be auto-installed by uv using declared script dependencies.Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In models/eqV2_tracegrad/test_fairchem_tracegrad.sh around lines 2-3, the script
sets an absolute PYTHONPATH to a local developer directory which breaks
reproducibility; remove the hard-coded path and either (a) compute the src path
relative to the script at runtime so the script works on other clones, or (b)
stop manipulating PYTHONPATH and require/install the package into the virtualenv
(pip install -e . or via your project’s dependency manager like pip/poetry/venv)
and run tests with python -m so dependencies are managed by the environment
rather than a hard-coded path.
| python test_fairchem_tracegrad_discovery.py \ | ||
| --out-path /data/fywang/code/matbench-discovery/pred_results/ \ | ||
| --model-name eqV2-small-dens-tracegrad \ | ||
| --optimizer FIRE \ | ||
| --cell-filter unit \ | ||
| --force-max 0.02 \ | ||
| --max-steps 500 \ | ||
| --num-jobs 10 \ | ||
| --device cuda \ | ||
| --checkpoint-path /data/fywang/code/fairchem-tracegrad-hanhai/checkpoints/2025-06-16-14-11-12/best_checkpoint.pt \ | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use module invocation and fix dangling backslashes before commented lines.
Also makes the script runnable from repo root.
- python test_fairchem_tracegrad_discovery.py \
+ uv run -m models.eqV2_tracegrad.test_fairchem_tracegrad_discovery \
--out-path /data/fywang/code/matbench-discovery/pred_results/ \
--model-name eqV2-small-dens-tracegrad \
--optimizer FIRE \
--cell-filter unit \
--force-max 0.02 \
--max-steps 500 \
--num-jobs 10 \
--device cuda \
- --checkpoint-path /data/fywang/code/fairchem-tracegrad-hanhai/checkpoints/2025-06-16-14-11-12/best_checkpoint.pt \
+ --checkpoint-path /data/fywang/code/fairchem-tracegrad-hanhai/checkpoints/2025-06-16-14-11-12/best_checkpoint.pt📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| python test_fairchem_tracegrad_discovery.py \ | |
| --out-path /data/fywang/code/matbench-discovery/pred_results/ \ | |
| --model-name eqV2-small-dens-tracegrad \ | |
| --optimizer FIRE \ | |
| --cell-filter unit \ | |
| --force-max 0.02 \ | |
| --max-steps 500 \ | |
| --num-jobs 10 \ | |
| --device cuda \ | |
| --checkpoint-path /data/fywang/code/fairchem-tracegrad-hanhai/checkpoints/2025-06-16-14-11-12/best_checkpoint.pt \ | |
| python -m models.eqV2_tracegrad.test_fairchem_tracegrad_discovery \ | |
| --out-path /data/fywang/code/matbench-discovery/pred_results/ \ | |
| --model-name eqV2-small-dens-tracegrad \ | |
| --optimizer FIRE \ | |
| --cell-filter unit \ | |
| --force-max 0.02 \ | |
| --max-steps 500 \ | |
| --num-jobs 10 \ | |
| --device cuda \ | |
| --checkpoint-path /data/fywang/code/fairchem-tracegrad-hanhai/checkpoints/2025-06-16-14-11-12/best_checkpoint.pt |
🤖 Prompt for AI Agents
In models/eqV2_tracegrad/test_fairchem_tracegrad.sh around lines 5–15, the
script uses a direct python script path with trailing backslashes that leave a
dangling line continuation before commented lines and hard-coded absolute paths;
change the invocation to module form (python -m
models.eqV2_tracegrad.test_fairchem_tracegrad_discovery) remove any trailing
backslashes that precede comments or the end of the command so the shell line
continuations are correct, and make file paths repo-root relative (or derive
them from the script location) so the script can be run from the repository
root.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
models/eqV2_tracegrad/join_fairchem_preds.py (4)
61-65: Fix the hard-coded baseline path.The hard-coded path to the baseline results file violates reproducibility. Based on the PR feedback, this needs to be parameterized.
73-74: Add missing lines=True parameter for JSON Lines file.The WBM computed structure entries file is a JSON Lines (.jsonl.gz) format that requires
lines=Truefor proper parsing.
98-107: Apply the processed MP corrections to the DataFrame.The MP2020 corrections are computed but never applied to the entries used for formation energy calculations.
127-136: Add compression parameters and fix df_bad output path.The output files need explicit compression settings and the bad results file should use a proper path structure.
🧹 Nitpick comments (3)
models/eqV2_tracegrad/join_fairchem_preds.py (3)
66-66: Replace fullwidth comma with standard comma.The comment contains an ambiguous fullwidth comma character that should be replaced with a standard comma for consistency.
- # 合并df_fairchem_ori中的pred_energy到df_fairchem，根据mat_id匹配 + # 合并df_fairchem_ori中的pred_energy到df_fairchem,根据mat_id匹配
40-41: Remove commented duplicate line.Line 40 contains commented code that duplicates line 41.
- # glob_pattern = f"{model_name}*.json.gz" glob_pattern = f"{model_name}*.json.gz"
93-93: Remove commented alternative energy assignment.This commented line appears to be debugging code that should be removed.
- # cse._energy = pred_energy_ori # added by fywang
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
models/eqV2_tracegrad/eqV2-s-dens-mp-ori.csv.gzis excluded by!**/*.gz
📒 Files selected for processing (1)
models/eqV2_tracegrad/join_fairchem_preds.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
models/eqV2_tracegrad/join_fairchem_preds.py (3)
matbench_discovery/enums.py (2)
DataFiles(472-573)MbdKey(46-123)matbench_discovery/data.py (1)
as_dict_handler(42-51)matbench_discovery/energy.py (1)
get_e_form_per_atom(149-182)
🪛 Ruff (0.12.2)
models/eqV2_tracegrad/join_fairchem_preds.py
66-66: Comment contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF003)
|
Hello Matbench Discovery Team, |
|
@TheSheng1103 we're not running your model in CI. the issues reported in Lint / prek are purely stylistic/formatting errors. for example, you seem to have a lot of unusual characters in your script. make sure you use UTF-8 encoding (and also fix the other lints) |
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
models/eqV2_tracegrad/test_fairchem_kappa.py (1)
288-299: JSON compression issue already flagged.This segment has the same JSON compression issue that was identified in the previous review. Files are written with
.json.gzextension butto_json()is called withoutcompression="gzip", which will cause read failures later.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
models/eqV2_tracegrad/test_fairchem_kappa.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
models/eqV2_tracegrad/test_fairchem_kappa.py (3)
matbench_discovery/enums.py (4)
metadata(379-388)DataFiles(503-604)path(580-604)base_dir(208-210)matbench_discovery/phonons/__init__.py (1)
check_imaginary_freqs(16-42)matbench_discovery/phonons/thermal_conductivity.py (4)
init_phono3py(116-158)get_fc2_and_freqs(161-194)calculate_fc3_set(74-113)calculate_conductivity(218-280)
🪛 GitHub Actions: Lint
models/eqV2_tracegrad/test_fairchem_kappa.py
[error] 239-239: hunted by diagnostics: unsupported-operator in test file during CI (likely due to path/union type handling).
🪛 Ruff (0.14.2)
models/eqV2_tracegrad/test_fairchem_kappa.py
192-192: Do not catch blind exception: Exception
(BLE001)
255-255: Do not catch blind exception: Exception
(BLE001)
270-270: Do not catch blind exception: Exception
(BLE001)
310-310: Do not catch blind exception: Exception
(BLE001)
327-327: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
330-330: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: pytest (windows-latest) / tests
- GitHub Check: pytest (ubuntu-latest) / tests
- GitHub Check: scripts (scripts/evals/discovery.py)
🔇 Additional comments (8)
models/eqV2_tracegrad/test_fairchem_kappa.py (8)
1-49: LGTM!The imports are well-organized and the
seed_everywherefunction correctly seeds all RNG sources for reproducibility.
51-68: LGTM!The constructor clearly stores all configuration parameters needed for the parallel workflow.
132-200: LGTM!The relaxation workflow is well-structured with proper error handling, symmetry checks, and detailed logging. The use of
optimizer.nstepson line 171 is correct.
202-263: LGTM!The force constant calculation logic correctly handles FC2/FC3 computation, imaginary frequency checks, and symmetry conditions. Error handling is comprehensive.
265-280: LGTM!The thermal conductivity calculation is straightforward with appropriate error handling and result aggregation.
301-324: LGTM!The result aggregation logic properly waits for all parallel jobs to complete before merging results, with appropriate error handling for file I/O.
337-353: LGTM!The output directory structure and Submitit executor configuration are properly set up for parallel job submission.
394-395: LGTM!The main guard correctly exposes the Typer CLI for command-line execution.
| def run(self, env_vars: None, job_number: int = 0) -> None: | ||
| # 设置 CUDA_VISIBLE_DEVICES 环境变量 | ||
| if env_vars: | ||
| os.environ.update(env_vars) | ||
|
|
There was a problem hiding this comment.
Fix incorrect type hint and remove Chinese comment.
Two issues:
- The type hint
env_vars: Noneis incorrect since the code checksif env_vars:on line 72. Should bedict[str, str] | None. - The Chinese comment on line 71 should be removed or translated to English per the maintainer's feedback on encoding issues.
Apply this diff:
- def run(self, env_vars: None, job_number: int = 0) -> None:
- # 设置 CUDA_VISIBLE_DEVICES 环境变量
+ def run(self, env_vars: dict[str, str] | None = None, job_number: int = 0) -> None:
+ # Set CUDA_VISIBLE_DEVICES environment variable
if env_vars:
os.environ.update(env_vars)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def run(self, env_vars: None, job_number: int = 0) -> None: | |
| # 设置 CUDA_VISIBLE_DEVICES 环境变量 | |
| if env_vars: | |
| os.environ.update(env_vars) | |
| def run(self, env_vars: dict[str, str] | None = None, job_number: int = 0) -> None: | |
| # Set CUDA_VISIBLE_DEVICES environment variable | |
| if env_vars: | |
| os.environ.update(env_vars) |
🤖 Prompt for AI Agents
In models/eqV2_tracegrad/test_fairchem_kappa.py around lines 70 to 74, the
method signature currently uses the incorrect type hint env_vars: None and
contains a Chinese comment; change the signature to env_vars: dict[str, str] |
None and remove (or replace with an English comment) the Chinese comment so the
code correctly types optional environment variable mappings and avoids non-ASCII
comment issues, leaving the body that checks if env_vars and updates os.environ
unchanged.
| save_dir = self.out_dir | ||
|
|
||
| # Log run parameters | ||
| timestamp = f"{datetime.now().astimezone():%Y-%m-%d %H:%M:%S}" |
There was a problem hiding this comment.
Variable name shadows imported function.
The local variable timestamp on line 110 shadows the imported timestamp function from matbench_discovery (line 37). While not currently causing issues in this code, it reduces readability and could lead to bugs if the function is needed later.
Apply this diff:
- timestamp = f"{datetime.now().astimezone():%Y-%m-%d %H:%M:%S}"
+ run_timestamp = f"{datetime.now().astimezone():%Y-%m-%d %H:%M:%S}"
run_params = {
- "timestamp": timestamp,
+ "timestamp": run_timestamp,Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In models/eqV2_tracegrad/test_fairchem_kappa.py around line 110, the local
variable named `timestamp` shadows the imported `timestamp` function from
matbench_discovery; rename the local variable (for example to
`current_timestamp` or `ts`) and update its usages on this line to avoid
shadowing and preserve readability.
| def run_kappa( | ||
| checkpoint_path: Annotated[Path, typer.Option(help="路径，包含 checkpoint.pt")], | ||
| out_path: Annotated[Path, typer.Option(help="Output path to write results files")], | ||
| model_name: Annotated[str, typer.Option(help="输出目录的子目录名")], | ||
| identifier: Annotated[str, typer.Option(help="运行标识，用于文件夹命名")], | ||
| atom_disp: Annotated[float, typer.Option(help="用于计算声子性质的位移距离")] = 0.03, | ||
| seed: Annotated[int, typer.Option(help="随机种子")] = 42, | ||
| num_jobs: Annotated[int, typer.Option(help="并行任务数")] = 10, | ||
| debug: Annotated[bool, typer.Option(help="调试模式,仅运行一个 job")] = False, | ||
| slurm_timeout: Annotated[int, typer.Option(help="slurm 超时时间(小时)")] = 80, | ||
| ) -> None: |
There was a problem hiding this comment.
Replace Chinese help text with English and fix fullwidth characters.
The maintainer specifically flagged these lint errors (RUF001, RUF003) caused by fullwidth characters in Chinese text. All help strings must use English with standard ASCII punctuation.
Apply this diff:
def run_kappa(
- checkpoint_path: Annotated[Path, typer.Option(help="路径，包含 checkpoint.pt")],
+ checkpoint_path: Annotated[Path, typer.Option(help="Path containing checkpoint.pt")],
out_path: Annotated[Path, typer.Option(help="Output path to write results files")],
- model_name: Annotated[str, typer.Option(help="输出目录的子目录名")],
- identifier: Annotated[str, typer.Option(help="运行标识，用于文件夹命名")],
- atom_disp: Annotated[float, typer.Option(help="用于计算声子性质的位移距离")] = 0.03,
- seed: Annotated[int, typer.Option(help="随机种子")] = 42,
- num_jobs: Annotated[int, typer.Option(help="并行任务数")] = 10,
- debug: Annotated[bool, typer.Option(help="调试模式,仅运行一个 job")] = False,
- slurm_timeout: Annotated[int, typer.Option(help="slurm 超时时间(小时)")] = 80,
+ model_name: Annotated[str, typer.Option(help="Subdirectory name for output")],
+ identifier: Annotated[str, typer.Option(help="Run identifier for folder naming")],
+ atom_disp: Annotated[float, typer.Option(help="Displacement distance for phonon calculations")] = 0.03,
+ seed: Annotated[int, typer.Option(help="Random seed")] = 42,
+ num_jobs: Annotated[int, typer.Option(help="Number of parallel jobs")] = 10,
+ debug: Annotated[bool, typer.Option(help="Debug mode, run only one job")] = False,
+ slurm_timeout: Annotated[int, typer.Option(help="SLURM timeout in hours")] = 80,
) -> None:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def run_kappa( | |
| checkpoint_path: Annotated[Path, typer.Option(help="路径，包含 checkpoint.pt")], | |
| out_path: Annotated[Path, typer.Option(help="Output path to write results files")], | |
| model_name: Annotated[str, typer.Option(help="输出目录的子目录名")], | |
| identifier: Annotated[str, typer.Option(help="运行标识，用于文件夹命名")], | |
| atom_disp: Annotated[float, typer.Option(help="用于计算声子性质的位移距离")] = 0.03, | |
| seed: Annotated[int, typer.Option(help="随机种子")] = 42, | |
| num_jobs: Annotated[int, typer.Option(help="并行任务数")] = 10, | |
| debug: Annotated[bool, typer.Option(help="调试模式,仅运行一个 job")] = False, | |
| slurm_timeout: Annotated[int, typer.Option(help="slurm 超时时间(小时)")] = 80, | |
| ) -> None: | |
| def run_kappa( | |
| checkpoint_path: Annotated[Path, typer.Option(help="Path containing checkpoint.pt")], | |
| out_path: Annotated[Path, typer.Option(help="Output path to write results files")], | |
| model_name: Annotated[str, typer.Option(help="Subdirectory name for output")], | |
| identifier: Annotated[str, typer.Option(help="Run identifier for folder naming")], | |
| atom_disp: Annotated[float, typer.Option(help="Displacement distance for phonon calculations")] = 0.03, | |
| seed: Annotated[int, typer.Option(help="Random seed")] = 42, | |
| num_jobs: Annotated[int, typer.Option(help="Number of parallel jobs")] = 10, | |
| debug: Annotated[bool, typer.Option(help="Debug mode, run only one job")] = False, | |
| slurm_timeout: Annotated[int, typer.Option(help="SLURM timeout in hours")] = 80, | |
| ) -> None: |
🧰 Tools
🪛 Ruff (0.14.2)
327-327: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
330-330: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
🤖 Prompt for AI Agents
In models/eqV2_tracegrad/test_fairchem_kappa.py around lines 326-336, the
Annotated typer.Option help strings currently contain Chinese text and fullwidth
punctuation which triggered lint errors (RUF001, RUF003); replace each help
string with concise English equivalents using standard ASCII characters and
punctuation (for example "Path containing checkpoint.pt", "Output path to write
results files", "Subdirectory name for output", "Run identifier for naming
folders", "Displacement distance used to compute phonon properties", "Random
seed", "Number of parallel jobs", "Debug mode: run only one job", "Slurm timeout
(hours)"), and ensure all punctuation is ASCII (commas, parentheses, hyphens)
and no fullwidth characters remain.
| # 继续填充函数 | ||
| if debug: | ||
| # 调试模式,只运行一个任务,使用GPU 0 | ||
| env_vars = {"CUDA_VISIBLE_DEVICES": "0"} | ||
| job = executor.submit( | ||
| KappaSRMERunner( | ||
| seed=seed, | ||
| model_dir=checkpoint_path, | ||
| out_dir=out_path, | ||
| save_name=model_name, | ||
| identifier=identifier, | ||
| atom_disp=atom_disp, | ||
| num_jobs=1, # 设置为0处理前3个样本 | ||
| ).run, | ||
| env_vars=env_vars, | ||
| job_number=0, | ||
| ) |
There was a problem hiding this comment.
Remove Chinese comment and fix fullwidth comma.
Line 356 contains a Chinese comment with fullwidth characters that must be fixed per the maintainer's lint feedback.
Apply this diff:
- # 继续填充函数
+ # Submit jobs
if debug:
- # 调试模式,只运行一个任务,使用GPU 0
+ # Debug mode: run a single task using GPU 0
env_vars = {"CUDA_VISIBLE_DEVICES": "0"}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # 继续填充函数 | |
| if debug: | |
| # 调试模式,只运行一个任务,使用GPU 0 | |
| env_vars = {"CUDA_VISIBLE_DEVICES": "0"} | |
| job = executor.submit( | |
| KappaSRMERunner( | |
| seed=seed, | |
| model_dir=checkpoint_path, | |
| out_dir=out_path, | |
| save_name=model_name, | |
| identifier=identifier, | |
| atom_disp=atom_disp, | |
| num_jobs=1, # 设置为0处理前3个样本 | |
| ).run, | |
| env_vars=env_vars, | |
| job_number=0, | |
| ) | |
| # Submit jobs | |
| if debug: | |
| # Debug mode: run a single task using GPU 0 | |
| env_vars = {"CUDA_VISIBLE_DEVICES": "0"} | |
| job = executor.submit( | |
| KappaSRMERunner( | |
| seed=seed, | |
| model_dir=checkpoint_path, | |
| out_dir=out_path, | |
| save_name=model_name, | |
| identifier=identifier, | |
| atom_disp=atom_disp, | |
| num_jobs=1, # 设置为0处理前3个样本 | |
| ).run, | |
| env_vars=env_vars, | |
| job_number=0, | |
| ) |
🤖 Prompt for AI Agents
In models/eqV2_tracegrad/test_fairchem_kappa.py around lines 354 to 370, remove
the Chinese inline comment on line 356 and replace the fullwidth comma with an
ASCII comma in that line; ensure the remaining comment (if needed) is in English
and uses standard ASCII punctuation, e.g., change "设置为0处理前3个样本" to an English
comment or delete it and replace the fullwidth comma "，" with "," so the file
passes linting.
| else: | ||
| # 正常模式,提交多个任务 | ||
| jobs = [] | ||
| with executor.batch(): | ||
| for job_number in range(num_jobs): | ||
| # 为每个任务分配不同的 GPU | ||
| env_vars = {"CUDA_VISIBLE_DEVICES": str(job_number)} | ||
| job = executor.submit( | ||
| KappaSRMERunner( | ||
| seed=seed, | ||
| model_dir=checkpoint_path, | ||
| out_dir=out_path, | ||
| save_name=model_name, | ||
| identifier=identifier, | ||
| atom_disp=atom_disp, | ||
| num_jobs=num_jobs, | ||
| ).run, | ||
| env_vars=env_vars, | ||
| job_number=job_number, | ||
| ) | ||
| jobs.append(job) |
There was a problem hiding this comment.
Remove Chinese comment and fix GPU assignment logic.
Two issues:
- Line 372 contains a Chinese comment that should be in English.
- Line 377 assigns GPU by
job_numberdirectly, which will fail ifnum_jobsexceeds available GPUs. Should distribute jobs across available devices.
Apply this diff:
else:
- # 正常模式,提交多个任务
+ # Normal mode: submit multiple tasks
jobs = []
with executor.batch():
for job_number in range(num_jobs):
- # 为每个任务分配不同的 GPU
- env_vars = {"CUDA_VISIBLE_DEVICES": str(job_number)}
+ # Assign GPU by round-robin across available devices
+ # Adjust num_gpus based on your cluster configuration
+ num_gpus = 8 # or pass as parameter
+ env_vars = {"CUDA_VISIBLE_DEVICES": str(job_number % num_gpus)}
job = executor.submit(Alternatively, remove the CUDA_VISIBLE_DEVICES assignment entirely and let the cluster scheduler handle GPU allocation, which is more robust:
for job_number in range(num_jobs):
- # 为每个任务分配不同的 GPU
- env_vars = {"CUDA_VISIBLE_DEVICES": str(job_number)}
+ env_vars = None
job = executor.submit(📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| else: | |
| # 正常模式,提交多个任务 | |
| jobs = [] | |
| with executor.batch(): | |
| for job_number in range(num_jobs): | |
| # 为每个任务分配不同的 GPU | |
| env_vars = {"CUDA_VISIBLE_DEVICES": str(job_number)} | |
| job = executor.submit( | |
| KappaSRMERunner( | |
| seed=seed, | |
| model_dir=checkpoint_path, | |
| out_dir=out_path, | |
| save_name=model_name, | |
| identifier=identifier, | |
| atom_disp=atom_disp, | |
| num_jobs=num_jobs, | |
| ).run, | |
| env_vars=env_vars, | |
| job_number=job_number, | |
| ) | |
| jobs.append(job) | |
| else: | |
| # Normal mode: submit multiple tasks | |
| jobs = [] | |
| with executor.batch(): | |
| for job_number in range(num_jobs): | |
| # Assign GPU by round-robin across available devices | |
| # Adjust num_gpus based on your cluster configuration | |
| num_gpus = 8 # or pass as parameter | |
| env_vars = {"CUDA_VISIBLE_DEVICES": str(job_number % num_gpus)} | |
| job = executor.submit( | |
| KappaSRMERunner( | |
| seed=seed, | |
| model_dir=checkpoint_path, | |
| out_dir=out_path, | |
| save_name=model_name, | |
| identifier=identifier, | |
| atom_disp=atom_disp, | |
| num_jobs=num_jobs, | |
| ).run, | |
| env_vars=env_vars, | |
| job_number=job_number, | |
| ) | |
| jobs.append(job) |
🤖 Prompt for AI Agents
In models/eqV2_tracegrad/test_fairchem_kappa.py around lines 371 to 391, replace
the Chinese inline comment with an English one and fix GPU assignment: do not
set CUDA_VISIBLE_DEVICES to job_number directly (fails when num_jobs > available
GPUs); either remove the CUDA_VISIBLE_DEVICES env_var entirely to let the
scheduler handle allocation, or compute device_id = job_number % available_gpus
(detected via torch.cuda.device_count() if torch is available or by parsing
CUDA_VISIBLE_DEVICES env var as a fallback) and set
env_vars={"CUDA_VISIBLE_DEVICES": str(device_id)} before submitting each job.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (7)
models/eqV2_tracegrad/test_fairchem_kappa_debug.py (1)
289-289: Write gzipped outputs for .json.gz files.This issue was already flagged in a previous review.
pd.DataFrame.to_json()does not infer compression from the filename extension. You must explicitly specifycompression="gzip"to actually compress the output.Apply this diff to fix the compression issue:
- df_kappa.reset_index().to_json(json_path) + df_kappa.reset_index().to_json(json_path, compression="gzip", orient="records")And at line 315:
- combined_df.to_json(save_dir / "kappa-103-FIRE.json.gz") + combined_df.to_json( + save_dir / "kappa-103-FIRE.json.gz", compression="gzip", orient="records" + )Also applies to: 315-315
models/eqV2_tracegrad/join_fairchem_preds.py (2)
60-70: Hard-coded baseline file path still present.The baseline CSV is loaded from a hard-coded relative path without directory context. This assumes the script runs from a specific working directory and breaks reproducibility.
As noted in previous reviews, make this an optional CLI parameter with a default value or use a project-relative path.
97-106: Corrected entries must be written back to the DataFrame.When MP2020 corrections are applied, the processed entries are validated but never replace the uncorrected entries in
df_fairchem[computed_structure_entry_key]. This means formation energies computed later still use uncorrected values.Apply the fix from the previous review:
if apply_mp_corrections is True: # apply energy corrections processed = MaterialsProject2020Compatibility().process_entries( df_fairchem[computed_structure_entry_key], verbose=True, clean=True ) if len(processed) != len(df_fairchem): raise ValueError( f"not all entries processed: {len(processed)=} {len(df_fairchem)=}" ) + # Replace uncorrected entries with corrected ones in the same order + df_fairchem[computed_structure_entry_key] = processedmodels/eqV2_tracegrad/test_fairchem_tracegrad_discovery.py (4)
3-4: Remove hard-coded sys.path manipulation.This hard-coded absolute path breaks reproducibility and was flagged in previous reviews. The maintainer recommended declaring script dependencies so others can run the code.
Remove these lines and ensure FAIRChem is installed in the environment or declared as a script dependency per the maintainer's recommendation.
33-36: Use DataFiles enum instead of hard-coded database path.This hard-coded absolute path was flagged in previous reviews. The recommended solution was to use
DataFiles.wbm_initial_atoms.path.Apply the fix from the previous review to use the shared DataFiles enum.
54-58: Remove hard-coded path manipulation from worker processes.This method replicates the same hard-coded sys.path issue from lines 3-4 for parallel workers.
Remove this method entirely once FAIRChem is properly installed in the environment.
261-261: Fix invalid Submitit parameter name.
slurm_gpus_per_taskis not a valid Submitit parameter and will be ignored. This was flagged in previous reviews.Apply this diff:
cpus_per_task=1, - slurm_gpus_per_task=1, # 每个任务使用一个 GPU + gpus_per_task=1, # one GPU per task nodes=1,
🧹 Nitpick comments (1)
models/eqV2_tracegrad/join_fairchem_preds.py (1)
91-93: Remove unused noqa directives.The
# noqa:SLF001comments are unnecessary since SLF001 is not enabled in your linter configuration.Apply this diff:
- cse._energy = pred_energy # noqa:SLF001 + cse._energy = pred_energy # cse._energy = pred_energy_ori # added by fywang - cse._structure = mlip_struct # noqa:SLF001 + cse._structure = mlip_struct
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
models/eqV2_tracegrad/join_fairchem_preds.py(1 hunks)models/eqV2_tracegrad/test_fairchem_kappa_debug.py(1 hunks)models/eqV2_tracegrad/test_fairchem_tracegrad_discovery.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-18T14:36:56.526Z
Learnt from: teddykoker
Repo: janosh/matbench-discovery PR: 276
File: models/nequix/nequix-mp-1.yml:33-36
Timestamp: 2025-08-18T14:36:56.526Z
Learning: In matbench-discovery model YAML files, test_task values can use more detailed specifications like "IS2RE-SR" (Initial Structure to Relaxed Energy - Structure Relaxation) even though the core Task enum in matbench_discovery/enums.py only defines "IS2RE". This "-SR" suffix is widely used across 30+ model configurations to indicate the structure relaxation variant.
Applied to files:
models/eqV2_tracegrad/test_fairchem_tracegrad_discovery.py
🧬 Code graph analysis (3)
models/eqV2_tracegrad/test_fairchem_kappa_debug.py (3)
matbench_discovery/enums.py (4)
metadata(379-388)DataFiles(503-604)path(580-604)base_dir(208-210)matbench_discovery/phonons/__init__.py (1)
check_imaginary_freqs(16-42)matbench_discovery/phonons/thermal_conductivity.py (4)
init_phono3py(116-158)get_fc2_and_freqs(161-194)calculate_fc3_set(74-113)calculate_conductivity(218-280)
models/eqV2_tracegrad/join_fairchem_preds.py (3)
matbench_discovery/enums.py (3)
DataFiles(503-604)MbdKey(46-122)path(580-604)matbench_discovery/data.py (1)
as_dict_handler(46-55)matbench_discovery/energy.py (1)
get_e_form_per_atom(149-182)
models/eqV2_tracegrad/test_fairchem_tracegrad_discovery.py (3)
matbench_discovery/enums.py (2)
MbdKey(46-122)Task(126-140)matbench_discovery/data.py (1)
as_dict_handler(46-55)matbench_discovery/plots.py (1)
wandb_scatter(703-729)
🪛 GitHub Actions: Lint
models/eqV2_tracegrad/join_fairchem_preds.py
[error] 87-87: RUF059 Unpacked variable pred_energy_ori is never used
🪛 Ruff (0.14.3)
models/eqV2_tracegrad/test_fairchem_kappa_debug.py
188-188: Do not catch blind exception: Exception
(BLE001)
251-251: Do not catch blind exception: Exception
(BLE001)
266-266: Do not catch blind exception: Exception
(BLE001)
310-310: Do not catch blind exception: Exception
(BLE001)
327-327: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
330-330: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
333-333: Unused function argument: num_jobs
(ARG001)
334-334: Unused function argument: debug
(ARG001)
models/eqV2_tracegrad/join_fairchem_preds.py
87-87: Unpacked variable pred_energy_ori is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
91-91: Unused noqa directive (non-enabled: SLF001)
Remove unused noqa directive
(RUF100)
93-93: Unused noqa directive (non-enabled: SLF001)
Remove unused noqa directive
(RUF100)
103-105: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: pytest (ubuntu-latest) / tests
- GitHub Check: pytest (windows-latest) / tests
- GitHub Check: scripts (scripts/evals/discovery.py)
🔇 Additional comments (5)
models/eqV2_tracegrad/test_fairchem_kappa_debug.py (5)
1-46: LGTM!The imports, copyright header, and
seed_everywhereutility are appropriate for the thermal conductivity workflow.
49-66: LGTM!The constructor correctly initializes all instance variables.
91-94: LGTM!The partitioning logic correctly splits work across jobs when
num_jobs > 0, and falls back to processing the first 3 samples for local debugging whennum_jobs <= 0.
188-196: Broad exception catching is acceptable here.While static analysis flags broad
Exceptioncatching, this pattern is appropriate for a workflow orchestration script where you want to log errors and continue processing remaining structures rather than failing entirely.Also applies to: 251-259, 266-274
364-365: LGTM!Standard Typer CLI entry point pattern.
| for row in tqdm( | ||
| df_fairchem.itertuples(), total=len(df_fairchem), desc="ML energies to CSEs" | ||
| ): | ||
| mat_id, struct_dict, pred_energy, pred_energy_ori, *_ = row |
There was a problem hiding this comment.
Fix unused variable triggering pipeline failure.
pred_energy_ori is unpacked but never used, causing the RUF059 lint failure.
Apply this diff:
- mat_id, struct_dict, pred_energy, pred_energy_ori, *_ = row
+ mat_id, struct_dict, pred_energy, _pred_energy_ori, *_ = row🧰 Tools
🪛 GitHub Actions: Lint
[error] 87-87: RUF059 Unpacked variable pred_energy_ori is never used
🪛 Ruff (0.14.3)
87-87: Unpacked variable pred_energy_ori is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
🤖 Prompt for AI Agents
In models/eqV2_tracegrad/join_fairchem_preds.py around line 87, the tuple
unpacking includes pred_energy_ori which is never used and triggers a RUF059
lint failure; remove pred_energy_ori from the unpacking (e.g., change to
"mat_id, struct_dict, pred_energy, *_ = row") or replace it with a single
underscore placeholder if you prefer explicit unused slot (e.g., "mat_id,
struct_dict, pred_energy, _ , *_ = row"), ensuring no other code references
pred_energy_ori.
| out_path = Path(out_path) | ||
| out_path = str(out_path) # 将其重新转换为字符串 | ||
|
|
||
| df_fairchem.select_dtypes("number").to_csv(out_path / f"{model_name}.csv.gz") | ||
| df_bad = df_fairchem[bad_mask].drop( | ||
| columns=[computed_structure_entry_key, struct_col] | ||
| ) | ||
| df_bad[MbdKey.e_form_dft] = df_wbm[MbdKey.e_form_dft] | ||
| df_bad.to_csv(f"{out_path}-bad.csv") | ||
|
|
||
| df_fairchem.reset_index().to_json( | ||
| out_path / f"{model_name}.json.gz", default_handler=as_dict_handler | ||
| ) |
There was a problem hiding this comment.
Critical type error: remove string conversion that breaks Path operations.
Line 125 converts out_path to a string, but lines 127 and 135 attempt to use the / operator which only works with Path objects. This will cause a runtime TypeError.
Apply this diff to remove the problematic string conversion:
out_path = Path(out_path)
- out_path = str(out_path) # 将其重新转换为字符串
- df_fairchem.select_dtypes("number").to_csv(out_path / f"{model_name}.csv.gz")
+ df_fairchem.select_dtypes("number").to_csv(
+ out_path / f"{model_name}.csv.gz",
+ compression="gzip",
+ )
df_bad = df_fairchem[bad_mask].drop(
columns=[computed_structure_entry_key, struct_col]
)
- df_bad[MbdKey.e_form_dft] = df_wbm[MbdKey.e_form_dft]
- df_bad.to_csv(f"{out_path}-bad.csv")
+ df_bad = df_bad.copy()
+ df_bad.loc[:, MbdKey.e_form_dft] = df_wbm.loc[df_bad.index, MbdKey.e_form_dft]
+ df_bad.to_csv(
+ out_path / f"{model_name}-bad.csv.gz",
+ compression="gzip",
+ )
df_fairchem.reset_index().to_json(
- out_path / f"{model_name}.json.gz", default_handler=as_dict_handler
+ out_path / f"{model_name}.json.gz",
+ compression="gzip",
+ default_handler=as_dict_handler,
)🤖 Prompt for AI Agents
In models/eqV2_tracegrad/join_fairchem_preds.py around lines 124-136, the code
converts out_path to a string which breaks subsequent Path operations using the
/ operator; remove the line that casts out_path to str so out_path remains a
Path, and ensure any functions that expect a string (if any) receive
str(out_path) explicitly at the call site instead of changing out_path itself.
| checkpoint_path: Annotated[Path, typer.Option(help="路径，包含 checkpoint.pt")], | ||
| out_path: Annotated[Path, typer.Option(help="Output path to write results files")], | ||
| model_name: Annotated[str, typer.Option(help="输出目录的子目录名")], | ||
| identifier: Annotated[str, typer.Option(help="运行标识，用于文件夹命名")], | ||
| atom_disp: Annotated[float, typer.Option(help="用于计算声子性质的位移距离")] = 0.03, | ||
| seed: Annotated[int, typer.Option(help="随机种子")] = 42, | ||
| num_jobs: Annotated[int, typer.Option(help="并行任务数")] = 10, | ||
| debug: Annotated[bool, typer.Option(help="调试模式,仅运行一个 job")] = False, | ||
| slurm_timeout: Annotated[int, typer.Option(help="slurm 超时时间(小时)")] = 80, |
There was a problem hiding this comment.
Fix fullwidth commas in help strings.
The maintainer explicitly requested fixing UTF-8 encoding issues and fullwidth characters. These Chinese help strings contain fullwidth commas (，) that should be replaced with regular commas (,) to pass lint checks.
Apply this diff to replace fullwidth commas:
- checkpoint_path: Annotated[Path, typer.Option(help="路径，包含 checkpoint.pt")],
+ checkpoint_path: Annotated[Path, typer.Option(help="路径, 包含 checkpoint.pt")],
out_path: Annotated[Path, typer.Option(help="Output path to write results files")],
model_name: Annotated[str, typer.Option(help="输出目录的子目录名")],
- identifier: Annotated[str, typer.Option(help="运行标识，用于文件夹命名")],
+ identifier: Annotated[str, typer.Option(help="运行标识, 用于文件夹命名")],
atom_disp: Annotated[float, typer.Option(help="用于计算声子性质的位移距离")] = 0.03,
seed: Annotated[int, typer.Option(help="随机种子")] = 42,
num_jobs: Annotated[int, typer.Option(help="并行任务数")] = 10,
- debug: Annotated[bool, typer.Option(help="调试模式,仅运行一个 job")] = False,
+ debug: Annotated[bool, typer.Option(help="调试模式, 仅运行一个 job")] = False,
slurm_timeout: Annotated[int, typer.Option(help="slurm 超时时间(小时)")] = 80,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| checkpoint_path: Annotated[Path, typer.Option(help="路径，包含 checkpoint.pt")], | |
| out_path: Annotated[Path, typer.Option(help="Output path to write results files")], | |
| model_name: Annotated[str, typer.Option(help="输出目录的子目录名")], | |
| identifier: Annotated[str, typer.Option(help="运行标识，用于文件夹命名")], | |
| atom_disp: Annotated[float, typer.Option(help="用于计算声子性质的位移距离")] = 0.03, | |
| seed: Annotated[int, typer.Option(help="随机种子")] = 42, | |
| num_jobs: Annotated[int, typer.Option(help="并行任务数")] = 10, | |
| debug: Annotated[bool, typer.Option(help="调试模式,仅运行一个 job")] = False, | |
| slurm_timeout: Annotated[int, typer.Option(help="slurm 超时时间(小时)")] = 80, | |
| checkpoint_path: Annotated[Path, typer.Option(help="路径, 包含 checkpoint.pt")], | |
| out_path: Annotated[Path, typer.Option(help="Output path to write results files")], | |
| model_name: Annotated[str, typer.Option(help="输出目录的子目录名")], | |
| identifier: Annotated[str, typer.Option(help="运行标识, 用于文件夹命名")], | |
| atom_disp: Annotated[float, typer.Option(help="用于计算声子性质的位移距离")] = 0.03, | |
| seed: Annotated[int, typer.Option(help="随机种子")] = 42, | |
| num_jobs: Annotated[int, typer.Option(help="并行任务数")] = 10, | |
| debug: Annotated[bool, typer.Option(help="调试模式, 仅运行一个 job")] = False, | |
| slurm_timeout: Annotated[int, typer.Option(help="slurm 超时时间(小时)")] = 80, |
🧰 Tools
🪛 Ruff (0.14.3)
327-327: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
330-330: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
333-333: Unused function argument: num_jobs
(ARG001)
334-334: Unused function argument: debug
(ARG001)
🤖 Prompt for AI Agents
In models/eqV2_tracegrad/test_fairchem_kappa_debug.py around lines 327 to 335,
several Chinese help strings use fullwidth commas (，); replace each fullwidth
comma with a standard ASCII comma (,) in the typer.Option help text for
checkpoint_path, out_path, model_name, identifier, atom_disp, seed, num_jobs,
debug, and slurm_timeout so the file contains only regular commas and passes
UTF-8/lint checks.
| num_jobs: Annotated[int, typer.Option(help="并行任务数")] = 10, | ||
| debug: Annotated[bool, typer.Option(help="调试模式,仅运行一个 job")] = False, |
There was a problem hiding this comment.
Remove unused function parameters.
The num_jobs and debug parameters are declared but never used in the function body. Line 358 hardcodes num_jobs=1 instead of using the parameter value, and debug is never referenced.
Either use these parameters as intended or remove them from the function signature. If they should control the behavior, apply this diff:
job = KappaSRMERunner(
seed=seed,
model_dir=checkpoint_path,
out_dir=out_path,
save_name=model_name,
identifier=identifier,
atom_disp=atom_disp,
- num_jobs=1, # 设置为0处理前3个样本
+ num_jobs=0 if debug else num_jobs,
)Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.14.3)
333-333: Unused function argument: num_jobs
(ARG001)
334-334: Unused function argument: debug
(ARG001)
🤖 Prompt for AI Agents
In models/eqV2_tracegrad/test_fairchem_kappa_debug.py around lines 333-334 and
reference to line 358, the function declares parameters num_jobs and debug but
never uses them (num_jobs is hardcoded to 1 at line 358 and debug is unused);
either remove them or wire them into behavior: replace the hardcoded num_jobs=1
with the num_jobs parameter and use the debug flag to override/force single-job
behavior when True (e.g., if debug: num_jobs = 1), or if you prefer to remove
them, delete both parameters from the signature and any related references;
ensure function behavior and any callers are updated accordingly.
| save_name=model_name, | ||
| identifier=identifier, | ||
| atom_disp=atom_disp, | ||
| num_jobs=1, # 设置为0处理前3个样本 |
There was a problem hiding this comment.
Clarify debug mode logic.
The comment states "设置为0处理前3个样本" (set to 0 to process first 3 samples), but the code sets num_jobs=1. Based on the logic at lines 91-94, num_jobs=0 triggers debug mode (first 3 samples), while num_jobs > 0 partitions across jobs.
The hardcoded value and comment are inconsistent with the declared debug parameter. See the suggestion in the previous comment to use the debug parameter properly.
🤖 Prompt for AI Agents
In models/eqV2_tracegrad/test_fairchem_kappa_debug.py around line 358, the
inline comment claims "设置为0处理前3个样本" but the code sets num_jobs=1, which
conflicts with the debug-mode logic at lines 91–94 where num_jobs==0 enables the
first-3-samples debug path; update the code so num_jobs is derived from the
debug parameter (e.g., if debug: num_jobs=0 else keep configured value) or
explicitly set num_jobs=0 when intending debug, and correct the comment to match
the actual behavior.
CompRhys
changed the title
janosh
force-pushed
the
main
branch
2 times, most recently
from
ff888ab to
93cc690
Compare
janosh
force-pushed
the
main
branch
5 times, most recently
from
76b10f0 to
e069cc4
Compare
janosh
force-pushed
the
main
branch
2 times, most recently
from
441c1f6 to
d2838a7
Compare
janosh
force-pushed
the
main
branch
5 times, most recently
from
11b8e76 to
1e4ecf1
Compare
janosh
force-pushed
the
main
branch
2 times, most recently
from
610666a to
0edb8a4
Compare
janosh
force-pushed
the
main
branch
3 times, most recently
from
4c72f3f to
dc6b114
Compare
3 participants
Hello Matbench Discovery Team,
First and foremost, I would like to express my sincere gratitude to you for your incredible efforts in building and maintaining the Matbench Discovery benchmark.Your work has enabled the machine learning models in the field of materials science to continuously progress and improve.
Our team recently trained a conservative model (eqV2_tracegrad) on the MPtrj training set, and we believe it may enhance the performance of the tasks you listed. We would be delighted to incorporate it into your benchmark tests.
I will add all the required files outlined in the contribution guide, please let me know if you need further information.
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.Model.<model_variant>enum on theModelenum inenums.py.<yyyy-mm-dd>-<model_variant>-preds.csv.gz).<yyyy-mm-dd>-wbm-IS2RE-FIRE.jsonl.gz). JSON Lines allows fast loading of small numbers of structures withpandas.read_json(lines=True, nrows=100)for inspection.<yyyy-mm-dd>-kappa-103-FIRE-<values-of-dist|fmax|symprec>.gz).<yyyy-mm-dd>-diatomics.json.gz).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.test_<arch_name>_<task>.pyfortaskindiscovery,kappa,diatomics) that generated the prediction files.just prepare-model-submission <model_variant>as described in the contributing guide to check metadata and generate the plots needed for submission.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
New Features
Chores