Jake update#51
Conversation
📝 WalkthroughWalkthroughAdds MLIP extxyz ingestion and pipeline integration, introduces shared parallel job and logging infrastructure, refactors calculator parsers, updates terminal output handling, and adds fixtures, tests, dependencies, and documentation. ChangesMLIP Pipeline and Parsing
Validation and Package Support
Estimated code review effort: 5 (Critical) | ~100 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant moldscript_main
participant run_file_jobs
participant CalculatorParser
participant mlip
participant get_df
CLI->>moldscript_main: parse MLIP and worker options
moldscript_main->>run_file_jobs: dispatch calculator parsing jobs
run_file_jobs->>CalculatorParser: parse source files
CalculatorParser-->>run_file_jobs: return descriptors and CPU times
moldscript_main->>mlip: parse configured extxyz states
mlip-->>moldscript_main: return file_data
moldscript_main->>get_df: write molecule, atom, and bond CSV files
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aec37f9578
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| entry["atom"]["frad"] = frad | ||
| entry["atom"]["charges_neutral"] = q_neutral | ||
|
|
||
| self.data_dict[key] = entry |
There was a problem hiding this comment.
When MLIP is combined with any earlier module in the same CLI run, main() passes the accumulated data_dicts into mlip, but this assignment replaces the whole molecule entry for matching keys. For extxyz files whose source_file stem matches the Gaussian-derived key, descriptors already parsed from opt/SPC/NMR/NBO/charges are silently dropped from the final CSVs and only the MLIP fields remain; merge into the existing nested mol/atom/bond dicts instead of overwriting the entry.
Useful? React with 👍 / 👎.
| source = header.get("source_file", "") | ||
| if source: | ||
| return Path(source).stem |
There was a problem hiding this comment.
When extxyz headers contain state-specific source_file values such as foo_neutral_vertical.com, foo_anion_Nplus1_vertical.com, and foo_cation_Nminus1_vertical.com, this early return bypasses the suffix stripping below. The three supplied state files then land under separate molecule keys, so each states dict is missing the other charge states and the vertical IE/EA/Fukui outputs become NaN; apply the same suffix normalization to the source stem before returning it.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
tests/test_moldscript.py (1)
184-242: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a merge-conflict regression test.
These tests validate the standalone
mlipparser output well, but none exercisemlip(..., data_dict=<pre-populated dict>)where the same key already hasmol/atom/bonddata from another stage (e.g.opt/charges). Given the overwrite behavior flagged inMLIP.py(Line 192), a regression test asserting pre-existing keys are preserved after merging would help catch this class of bug going forward.🤖 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 `@tests/test_moldscript.py` around lines 184 - 242, Add a regression test for the merge path in mlip when data_dict already contains an entry for the same filename. In tests/test_moldscript.py, exercise mlip(..., data_dict=<pre-populated dict>) with an existing key that already has mol/atom/bond data from another stage, then assert the parser merge preserves the pre-existing values instead of overwriting them. Use build_arbr141_parser, mlip, and the existing arbr141_wb97xd fixture shape as the reference points.
🤖 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 `@moldscript/charges.py`:
- Around line 62-73: Guard chg_data before accessing atomcharges in the charge
parsing flow, because parse_cc_data() can return None and the current
unconditional use in the charges handling block will raise AttributeError and
stop the batch. Update the logic around the atom charge assignment and the
subsequent get_atom_spins call in charges.py to first verify chg_data is present
(following the existing pattern used for cpu_time), and only then read
atomcharges and populate self.data_dict[filename]['atom'].
In `@moldscript/MLIP.py`:
- Around line 291-303: The _state_charges helper in MLIP.py silently pads or
truncates charge arrays when len(arr) does not match natoms, which can hide
state/geometry mismatches. Update _state_charges to emit a warning whenever the
atom charge array length differs from natoms, while keeping the existing
NaN-padding behavior; use the _state_charges method and
self.atom_charge_property to locate the mismatch path and make the warning
include the expected and actual lengths.
- Around line 196-206: The _read_extxyz method currently slices atom_lines
without checking that the file actually contains the declared number of atoms,
so truncated or malformed input can slip through silently. In _read_extxyz,
after building atom_lines from lines[2:2 + natoms], validate that its length
matches natoms and raise a clear ValueError when it does not; keep the check
close to the existing natoms/header parsing logic so the failure is reported
before _parse_properties_schema or downstream atomnos/coords construction.
- Around line 117-194: The per-key record in the MLIP aggregation loop is being
overwritten by assigning a new entry to self.data_dict[key], which drops any
existing mol, atom, bond, or CPU_time data already stored for that key. Update
the logic in the state-processing block inside the MLIP class so it merges into
the existing self.data_dict[key] entry in place, preserving previously populated
nested fields while adding the new MLIP values.
---
Nitpick comments:
In `@tests/test_moldscript.py`:
- Around line 184-242: Add a regression test for the merge path in mlip when
data_dict already contains an entry for the same filename. In
tests/test_moldscript.py, exercise mlip(..., data_dict=<pre-populated dict>)
with an existing key that already has mol/atom/bond data from another stage,
then assert the parser merge preserves the pre-existing values instead of
overwriting them. Use build_arbr141_parser, mlip, and the existing
arbr141_wb97xd fixture shape as the reference points.
🪄 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: fdea8659-82ff-4828-952c-a048c85a5471
⛔ Files ignored due to path filters (3)
moldscript/examples/spin_examples/A1a_cat_rad_opt.logis excluded by!**/*.logmoldscript/examples/spin_examples/A1b_cat_rad_opt.logis excluded by!**/*.logmoldscript/examples/spin_examples/A1c_cat_rad_opt.logis excluded by!**/*.log
📒 Files selected for processing (8)
moldscript/MLIP.pymoldscript/argument_parser.pymoldscript/charges.pymoldscript/examples/arbr/mlip/neutral/arbr141_wb97xd_neutral_vertical.extxyzmoldscript/examples/arbr/mlip/oxidized/arbr141_wb97xd_cation_Nminus1_vertical.extxyzmoldscript/examples/arbr/mlip/reduced/arbr141_wb97xd_anion_Nplus1_vertical.extxyzmoldscript/moldscript.pytests/test_moldscript.py
| for key, states in self.state_records.items(): | ||
| base_state = self._pick_base_state(states) | ||
| base_record = states[base_state] | ||
|
|
||
| symbols = base_record["symbols"] | ||
| coords = np.array(base_record["coords"], dtype=float) | ||
| atomnos = np.array([self.ptable.GetAtomicNumber(sym) for sym in symbols], dtype=int) | ||
|
|
||
| entry = { | ||
| "mol": {}, | ||
| "atom": {}, | ||
| "bond": {}, | ||
| "CPU_time": datetime.timedelta(0), | ||
| } | ||
|
|
||
| entry["atom"]["atomnos"] = atomnos | ||
| entry["bond"]["bond_length"] = self._bond_length_matrix(coords) | ||
|
|
||
| entry["mol"].setdefault("smiles", "") | ||
|
|
||
| # State energies | ||
| e_neutral_ev = self._state_energy_ev(states.get("neutral")) | ||
| e_reduced_ev = self._state_energy_ev(states.get("reduced")) | ||
| e_oxidized_ev = self._state_energy_ev(states.get("oxidized")) | ||
|
|
||
| if np.isfinite(e_neutral_ev): | ||
| entry["mol"]["scfenergy"] = e_neutral_ev * eV_to_hartree | ||
|
|
||
| else: | ||
| entry["mol"]["scfenergy"] = np.nan | ||
|
|
||
| if np.isfinite(e_oxidized_ev) and np.isfinite(e_neutral_ev): | ||
| ie_h = (e_oxidized_ev - e_neutral_ev) * eV_to_hartree | ||
| entry["mol"]["vertical_ie"] = ie_h | ||
| entry["mol"]["ionization_potential"] = ie_h | ||
| else: | ||
| entry["mol"]["vertical_ie"] = np.nan | ||
| entry["mol"]["ionization_potential"] = np.nan | ||
|
|
||
| if np.isfinite(e_reduced_ev) and np.isfinite(e_neutral_ev): | ||
| ea_h = (e_reduced_ev - e_neutral_ev) * eV_to_hartree | ||
| entry["mol"]["vertical_ea"] = ea_h | ||
| entry["mol"]["electron_affinity"] = ea_h | ||
| else: | ||
| entry["mol"]["vertical_ea"] = np.nan | ||
| entry["mol"]["electron_affinity"] = np.nan | ||
|
|
||
| # Dipole from the first state that provides it (neutral preferred). | ||
| dip = self._dipole_vector(states) | ||
| entry["mol"]["dipole_x"] = dip[0] | ||
| entry["mol"]["dipole_y"] = dip[1] | ||
| entry["mol"]["dipole_z"] = dip[2] | ||
| entry["mol"]["dipole"] = float(np.linalg.norm(dip)) if np.all(np.isfinite(dip)) else np.nan | ||
|
|
||
| q_neutral = self._state_charges(states.get("neutral"), len(atomnos)) | ||
| q_reduced = self._state_charges(states.get("reduced"), len(atomnos)) | ||
| q_oxidized = self._state_charges(states.get("oxidized"), len(atomnos)) | ||
|
|
||
| # Follow sign convention already used in fukui.py | ||
| if np.all(np.isfinite(q_neutral)) and np.all(np.isfinite(q_reduced)): | ||
| fplus = -1.0 * (q_reduced - q_neutral) | ||
| else: | ||
| fplus = np.full(len(atomnos), np.nan) | ||
|
|
||
| if np.all(np.isfinite(q_neutral)) and np.all(np.isfinite(q_oxidized)): | ||
| fminus = -1.0 * (q_neutral - q_oxidized) | ||
| else: | ||
| fminus = np.full(len(atomnos), np.nan) | ||
|
|
||
| frad = 0.5 * (fplus + fminus) | ||
| entry["atom"]["fplus"] = fplus | ||
| entry["atom"]["fminus"] = fminus | ||
| entry["atom"]["frad"] = frad | ||
| entry["atom"]["charges_neutral"] = q_neutral | ||
|
|
||
| self.data_dict[key] = entry | ||
|
|
||
| return self.data_dict |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file first, then inspect the surrounding code paths.
printf '\n== moldscript/MLIP.py outline ==\n'
ast-grep outline moldscript/MLIP.py --view expanded || true
printf '\n== moldscript/MLIP.py around the assignment ==\n'
sed -n '1,260p' moldscript/MLIP.py | cat -n
printf '\n== search for data_dict construction / reuse ==\n'
rg -n "data_dict|mlip\(" moldscript.py moldscript -g '!**/__pycache__/**' || true
printf '\n== inspect charges.py pattern mentioned in the comment ==\n'
if [ -f moldscript/charges.py ]; then
sed -n '1,240p' moldscript/charges.py | cat -n
fiRepository: patonlab/molDscript
Length of output: 33898
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== moldscript.py around the MLIP call ==\n'
sed -n '120,150p' moldscript/moldscript.py | cat -n
printf '\n== MLIP key normalization and base-state selection ==\n'
sed -n '260,340p' moldscript/MLIP.py | cat -n
printf '\n== utils.initiate_data_dict and record_cpu_time ==\n'
sed -n '130,210p' moldscript/utils.py | cat -n
sed -n '80,115p' moldscript/utils.py | cat -nRepository: patonlab/molDscript
Length of output: 9986
Merge MLIP fields into the existing entry
self.data_dict[key] = entry replaces the whole per-file record, so any earlier mol/atom/bond/CPU_time fields for the same key are dropped. Update the nested dicts in place instead of overwriting the entry.
🤖 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 `@moldscript/MLIP.py` around lines 117 - 194, The per-key record in the MLIP
aggregation loop is being overwritten by assigning a new entry to
self.data_dict[key], which drops any existing mol, atom, bond, or CPU_time data
already stored for that key. Update the logic in the state-processing block
inside the MLIP class so it merges into the existing self.data_dict[key] entry
in place, preserving previously populated nested fields while adding the new
MLIP values.
| def _read_extxyz(self, file_path): | ||
| lines = file_path.read_text(encoding="utf-8").splitlines() | ||
| if len(lines) < 2: | ||
| raise ValueError(f"Invalid extxyz file: {file_path}") | ||
|
|
||
| natoms = int(lines[0].strip()) | ||
| header_line = lines[1].strip() | ||
| header = self._parse_header_line(header_line) | ||
|
|
||
| schema = self._parse_properties_schema(header.get("Properties", "species:S:1:pos:R:3")) | ||
| atom_lines = lines[2 : 2 + natoms] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
No validation that the file actually contains natoms atom lines.
atom_lines = lines[2 : 2 + natoms] (Line 206) will silently yield fewer rows than natoms if the file is truncated/malformed, producing an atomnos/coords array shorter than the declared count with no error raised.
Consider asserting len(atom_lines) == natoms and raising a clear ValueError otherwise.
🤖 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 `@moldscript/MLIP.py` around lines 196 - 206, The _read_extxyz method currently
slices atom_lines without checking that the file actually contains the declared
number of atoms, so truncated or malformed input can slip through silently. In
_read_extxyz, after building atom_lines from lines[2:2 + natoms], validate that
its length matches natoms and raise a clear ValueError when it does not; keep
the check close to the existing natoms/header parsing logic so the failure is
reported before _parse_properties_schema or downstream atomnos/coords
construction.
| def _state_charges(self, record, natoms): | ||
| if record is None: | ||
| return np.full(natoms, np.nan) | ||
| values = record["atom_props"].get(self.atom_charge_property) | ||
| if values is None: | ||
| return np.full(natoms, np.nan) | ||
| arr = np.array(values, dtype=float) | ||
| if len(arr) == natoms: | ||
| return arr | ||
| padded = np.full(natoms, np.nan) | ||
| ncopy = min(natoms, len(arr)) | ||
| padded[:ncopy] = arr[:ncopy] | ||
| return padded |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Silent NaN-padding on atom-count mismatch.
If a state's charge array length differs from natoms, values are silently truncated/padded with NaN with no warning (Lines 300-303). This could mask a real geometry/state mismatch and quietly produce partially-NaN Fukui indices downstream.
Consider logging a warning when len(arr) != natoms so mismatches are visible rather than silently absorbed.
🤖 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 `@moldscript/MLIP.py` around lines 291 - 303, The _state_charges helper in
MLIP.py silently pads or truncates charge arrays when len(arr) does not match
natoms, which can hide state/geometry mismatches. Update _state_charges to emit
a warning whenever the atom charge array length differs from natoms, while
keeping the existing NaN-padding behavior; use the _state_charges method and
self.atom_charge_property to locate the mismatch path and make the warning
include the expected and actual lengths.
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (7)
moldscript/opt.py (3)
87-93: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse truthy/falsy checks for booleans.
Comparing directly to
FalseorTrueis an anti-pattern in Python. Usenot xtbandelseinstead.♻️ Proposed refactor
- if xtb == False: + if not xtb: # convert log to smiles opt_data = parse_cc_data(file_name, self.data[file_name]) cpu_times = opt_data.metadata.get('cpu_time') if hasattr(opt_data, 'metadata') else None self.module_cpu_seconds += cpu_times_seconds(cpu_times) record_cpu_time(self.data_dict, file_name, self.data[file_name], cpu_times) - elif xtb == True: + else:🤖 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 `@moldscript/opt.py` around lines 87 - 93, Update the conditional around xtb in the optimization flow to use `if not xtb` instead of comparing with False, and retain the existing fallback branch as else instead of explicitly comparing with True.
43-45: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove unnecessary f-string prefix.
The string does not contain any formatting placeholders.
♻️ Proposed refactor
self.args.log.write( - f"\nx Could not find files to obtain optimization information. Exiting program" + "\nx Could not find files to obtain optimization information. Exiting program" )🤖 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 `@moldscript/opt.py` around lines 43 - 45, Update the log message in the optimization flow to use a regular string literal instead of an f-string, preserving the existing message text and behavior.
60-72: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClean up unused variables and simplify Python constructs.
- The
mydictlambda is defined but never used and can be safely removed.- The
fprefix on the string at line 62 is unnecessary as there are no placeholders.- Retrieving the first value of the dictionary can be simplified to
next(iter(self.data.values()))to avoid creating an intermediate list.♻️ Proposed refactor
- mydict = lambda: defaultdict(mydict) - - self.args.log.write(f"-- Optimization Parameter Collection starting") + self.args.log.write("-- Optimization Parameter Collection starting") self.module_cpu_seconds = 0.0 if all(self.data_dict.get(file_name, {}).get("CPU_time") for file_name in self.data): self.module_cpu_seconds = sum( self.data_dict[file_name]["CPU_time"].total_seconds() for file_name in self.data ) report_job_progress(self.args.log, len(self.data), len(self.data)) return self.data_dict - test_file = self.data[list(self.data.keys())[0]] + test_file = next(iter(self.data.values()))🤖 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 `@moldscript/opt.py` around lines 60 - 72, In the relevant optimization setup, remove the unused mydict lambda, change the constant log message written to self.args.log to a normal string, and replace the list-based first-entry lookup with next(iter(self.data.values())) when assigning test_file. Preserve the surrounding optimization and progress-reporting behavior.moldscript/lowe.py (1)
72-79: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix copy-pasted comment.
The inline comment mentions the "atomic DataFrame", but this method is processing the bond DataFrame (
bond_df).♻️ Proposed fix
emit('Including only lowest-energy conformer bond data into {}'.format(ensemble_bond_csv), style="cyan") - # Map the weights to the atomic DataFrame based on 'filename' + # Map the weights to the bond DataFrame based on 'filename' weighted_df = bond_df[bond_df['filename'].isin(self.low_confs)] weighted_df['filename'] = [k.rsplit('_conf',1)[0] for k in weighted_df['filename']] columns_order = ['filename', 'atom1_idx', 'atom1', 'atom2_idx', 'atom2'] + [col for col in weighted_df.columns if col not in ['filename', 'atom1_idx', 'atom1', 'atom2_idx', 'atom2']] weighted_df = weighted_df[columns_order] weighted_df = weighted_df.round(4)🤖 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 `@moldscript/lowe.py` around lines 72 - 79, Update the inline comment above the `weighted_df` assignment to refer to the bond DataFrame (`bond_df`) instead of the atomic DataFrame, leaving the surrounding processing unchanged.moldscript/get_df.py (1)
194-195: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix indentation for readability.
The
append_run_logcall is indented with 8 spaces relative to theforloop instead of the conventional 4 spaces.♻️ Proposed fix
for prop in props: - append_run_log(f"\t- {prop}") + append_run_log(f"\t- {prop}")🤖 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 `@moldscript/get_df.py` around lines 194 - 195, Correct the indentation of the append_run_log call inside the for prop in props loop so it uses the conventional single nested-block indentation relative to the loop.moldscript/sterics.py (1)
23-23: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse truthiness check for boolean evaluation.
As suggested by static analysis, prefer
if vall:orif vall is not False:overif vall != False:.♻️ Proposed fix
- if vall != False: + if vall:🤖 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 `@moldscript/sterics.py` at line 23, Update the boolean condition in the surrounding logic to use a truthiness check (`if vall:`) instead of comparing `vall` with `False`, preserving the existing behavior for truthy values.Source: Linters/SAST tools
moldscript/files.py (1)
141-147: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse idiomatic truthiness checks to prevent
Nonestringification.As suggested by static analysis, prefer
not self.warn_suffixoverself.warn_suffix == False. Additionally, evaluating the truthiness ofsuffixrather than explicitly checkingsuffix != ''prevents a scenario wheresuffixisNone(evaluating toTrue) and becomes incorrectly stringified to"None".♻️ Proposed fix
if suffix: suffix = str(suffix).strip().lstrip("_") fullname = fullname.split("_" + suffix)[0] elif not self.warn_suffix: emit(f"Warning: no suffix provided for {self.calc}; matching will use each full filename stem.", style="yellow") emit(f"If {self.calc} filenames include a module tag, pass --suffix_{self.calc} so they match the optimization keys.", style="yellow") self.warn_suffix = True🤖 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 `@moldscript/files.py` around lines 141 - 147, Update the suffix handling in the surrounding filename-matching method to enter the normalization and splitting branch only when suffix is truthy, preventing None from being stringified; also replace the self.warn_suffix == False comparison with the idiomatic not self.warn_suffix check while preserving the existing warning behavior.Source: Linters/SAST tools
🤖 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 `@moldscript/argument_parser.py`:
- Around line 196-198: Update the success message in the terminal_success call
to replace the XXX documentation placeholder with the actual Moldscript
documentation URL, preserving the existing message wording and formatting.
- Around line 111-113: Update the case-insensitive option branch in the argument
parsing logic so it checks the lowercased key against var_dict but retrieves the
value from kwargs using the original key, preventing KeyError for differently
cased options.
In `@moldscript/fmo.py`:
- Around line 29-39: Update the electrophilicity calculation in the mol_values
computation so the denominator uses hardness, defined as half of the HOMO–LUMO
gap, while preserving the existing chemical_potential and reported gap values.
Ensure global_electrophilicity is calculated as μ²/(2η) and
global_nucleophilicity continues to use its reciprocal.
- Around line 25-26: Update the dipole calculation in the fmo.py flow to compute
the norm directly from fmo_data.moments[1] using np.linalg.norm, removing the
subtraction from fmo_data.moments[0] while preserving the reported scalar dipole
value.
In `@moldscript/fukui.py`:
- Around line 162-164: Update the job-building loop in fukui_data_dict() so raw
entries whose neutral baseline file is missing are detected and skipped before
calling resolve_data_key(). Preserve processing for entries with available
neutral files, and ensure missing-neutral cases reach the existing skip behavior
rather than aborting the run.
- Around line 31-35: Update the NPA section selection in the loop that populates
list_npop so start_npop uses the final detected NPA table, matching
scfenergies[-1], rather than list_npop[0]. Preserve the existing behavior when
no NPA section is found.
- Around line 103-106: Update the vertical_ea calculation in the mol_values
mapping to subtract reduced_data["energy"] from neutral_data["energy"], while
leaving the vertical_ie calculation unchanged.
In `@moldscript/moldscript.py`:
- Around line 160-161: Update main() to initialize first_read to None alongside
data_dicts before the input-selection branches, preventing the --link path from
raising NameError. Replace the direct False comparisons in the sterics condition
with a truthy check for args.volume or args.vall, and only invoke sterics when
those options are requested and first_read contains valid structural input
rather than None or an empty value.
In `@moldscript/utils.py`:
- Around line 519-524: Update resolve_data_key and filename_match_candidates so
calculator filenames are not matched by unrestricted prefix truncation. Only
strip the configured module suffix when deriving candidates, or reject
candidates that remove molecule-name components; ensure names such as
foo_bar_spc.log do not resolve to foo when foo_bar is unavailable.
- Around line 267-275: Update initialize_run_log and the related
_LOG_PATHS_INITIALIZED handling so each new run explicitly truncates the audit
log and clears any prior path-initialization state before writing provenance.
Ensure repeated main() invocations with the same output path start with an
independent log while preserving the existing run metadata and command-line
entries.
- Around line 635-639: Update bond_data_matrix so its coordinate-extraction
fallback catches only expected attribute, indexing, and conversion failures
(such as AttributeError, IndexError, TypeError, and ValueError), rather than
using a bare except that also intercepts process-control exceptions.
In `@README.md`:
- Line 43: Update the README option description near the output PREFIX entry to
list --no_mol, --no_atom, and --no_bond as CSV-output toggles, while documenting
--no_bond_filter separately as the bond-inclusion control; keep the wording
consistent with docs/source/README.rst.
---
Nitpick comments:
In `@moldscript/files.py`:
- Around line 141-147: Update the suffix handling in the surrounding
filename-matching method to enter the normalization and splitting branch only
when suffix is truthy, preventing None from being stringified; also replace the
self.warn_suffix == False comparison with the idiomatic not self.warn_suffix
check while preserving the existing warning behavior.
In `@moldscript/get_df.py`:
- Around line 194-195: Correct the indentation of the append_run_log call inside
the for prop in props loop so it uses the conventional single nested-block
indentation relative to the loop.
In `@moldscript/lowe.py`:
- Around line 72-79: Update the inline comment above the `weighted_df`
assignment to refer to the bond DataFrame (`bond_df`) instead of the atomic
DataFrame, leaving the surrounding processing unchanged.
In `@moldscript/opt.py`:
- Around line 87-93: Update the conditional around xtb in the optimization flow
to use `if not xtb` instead of comparing with False, and retain the existing
fallback branch as else instead of explicitly comparing with True.
- Around line 43-45: Update the log message in the optimization flow to use a
regular string literal instead of an f-string, preserving the existing message
text and behavior.
- Around line 60-72: In the relevant optimization setup, remove the unused
mydict lambda, change the constant log message written to self.args.log to a
normal string, and replace the list-based first-entry lookup with
next(iter(self.data.values())) when assigning test_file. Preserve the
surrounding optimization and progress-reporting behavior.
In `@moldscript/sterics.py`:
- Line 23: Update the boolean condition in the surrounding logic to use a
truthiness check (`if vall:`) instead of comparing `vall` with `False`,
preserving the existing behavior for truthy values.
🪄 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: c70d5f80-762a-4035-8dc4-974bbcbe2f5d
📒 Files selected for processing (22)
README.mddocs/source/README.rstmoldscript/MLIP.pymoldscript/__main__.pymoldscript/argument_parser.pymoldscript/boltz.pymoldscript/charges.pymoldscript/files.pymoldscript/fmo.pymoldscript/fukui.pymoldscript/get_df.pymoldscript/lowe.pymoldscript/min_max.pymoldscript/moldscript.pymoldscript/nbo.pymoldscript/nmr.pymoldscript/opt.pymoldscript/spc.pymoldscript/sterics.pymoldscript/utils.pyrequirements.txtsetup.py
🚧 Files skipped from review as they are similar to previous changes (1)
- moldscript/MLIP.py
| elif key.lower() in var_dict: | ||
| vars(options)[key.lower()] = kwargs[key.lower()] | ||
| else: |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Fix KeyError for case-insensitive option matching.
If a user provides an option with incorrect casing (e.g., --Workers 2), key.lower() in var_dict evaluates to True, but kwargs[key.lower()] will raise a KeyError because kwargs contains the original casing ("Workers"). Change the lookup to use the original key.
🐛 Proposed fix
elif key.lower() in var_dict:
- vars(options)[key.lower()] = kwargs[key.lower()]
+ vars(options)[key.lower()] = kwargs[key]
else:📝 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.
| elif key.lower() in var_dict: | |
| vars(options)[key.lower()] = kwargs[key.lower()] | |
| else: | |
| elif key.lower() in var_dict: | |
| vars(options)[key.lower()] = kwargs[key] | |
| else: |
🤖 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 `@moldscript/argument_parser.py` around lines 111 - 113, Update the
case-insensitive option branch in the argument parsing logic so it checks the
lowercased key against var_dict but retrieves the value from kwargs using the
original key, preventing KeyError for differently cased options.
| terminal_success( | ||
| f"o MOLDSCRIPT v {moldscript_version} is installed correctly! For more information about the available options, see the documentation in XXX" | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Replace documentation placeholder.
The help message contains a placeholder (XXX) for the documentation link. Please update it with the actual URL.
📝 Proposed fix
- f"o MOLDSCRIPT v {moldscript_version} is installed correctly! For more information about the available options, see the documentation in XXX"
+ f"o MOLDSCRIPT v {moldscript_version} is installed correctly! For more information about the available options, see the documentation at https://moldscript.readthedocs.io/"📝 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.
| terminal_success( | |
| f"o MOLDSCRIPT v {moldscript_version} is installed correctly! For more information about the available options, see the documentation in XXX" | |
| ) | |
| terminal_success( | |
| f"o MOLDSCRIPT v {moldscript_version} is installed correctly! For more information about the available options, see the documentation at https://moldscript.readthedocs.io/" | |
| ) |
🤖 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 `@moldscript/argument_parser.py` around lines 196 - 198, Update the success
message in the terminal_success call to replace the XXX documentation
placeholder with the actual Moldscript documentation URL, preserving the
existing message wording and formatting.
| fmo_data = cc.io.ccread(source_path) | ||
| dipole = np.sqrt(np.sum((fmo_data.moments[0] - fmo_data.moments[1]) ** 2, axis=0)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C3 'moments\[[012]\]' moldscript testsRepository: patonlab/molDscript
Length of output: 1447
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline moldscript/fmo.py --view expanded || true
echo '---'
cat -n moldscript/fmo.py | sed -n '1,220p'Repository: patonlab/molDscript
Length of output: 7039
🌐 Web query:
cclib data_notes moments[0] moments[1] dipole vector reference point
💡 Result:
In cclib, the moments attribute is a list of one-dimensional arrays representing the molecular multipole moments of a molecule [1][2]. This attribute follows a specific structure for its elements [3][1][2]: 1. The first element (moments) is the reference point used for the multipole expansion, which is typically the center of mass of the molecule [3][1][2]. 2. The second element (moments[3]) is the dipole moment vector [3][1][2]. This vector is expressed in units of Debyes (D) [1][2]. Subsequent elements in the moments list contain higher-order electrostatic multipole moments, such as the quadrupole moments (typically the third element, moments[1]), which are provided in lexicographical order (XX, XY, XZ, YY, YZ, ZZ) and measured in Buckinghams (B) [1][2]. Any further arrays in the list represent multipole moments of even higher rank, also in lexicographical order, expressed in units of D Å^(L-1) [1][2]. By default, cclib reports the moments from the final geometry or wavefunction printed in the output file [1][2]. If multiple sets of moments are printed during a job, such as during a geometry optimization, cclib provides the last set parsed [1][2].
Citations:
- 1: https://cclib.readthedocs.io/en/latest/data_notes.html
- 2: https://github.com/cclib/cclib/blob/master/doc/sphinx/data_notes.rst
- 3: https://cclib.github.io/data_notes.html
🌐 Web query:
site:cclib.readthedocs.io data_notes moments dipole moment vector moments[0] moments[1]
💡 Result:
In the context of the cclib computational chemistry library, the moments attribute contains the dipole moment vector and higher electrostatic multipole moments for a molecule [1]. This attribute is structured as a list of one-dimensional arrays [1]. Within this list, the indices refer to specific data: - moments: This is the reference point used for the multipole expansion, which is typically the center of mass [1]. - moments[1]: This contains the dipole moment vector, expressed in Debyes [1]. Any subsequent arrays in the moments list (e.g., moments[2], moments[3], etc.) contain higher-rank multipole moments, such as the molecular quadrupole moments (in Buckinghams) at index 2 [1].
Citations:
- 1: https://cclib.readthedocs.io/en/latest/data%5Fnotes.html
- 2: https://cclib.readthedocs.io/en/latest/data.html
- 3: https://cclib.readthedocs.io/en/latest/how_to_parse.html
Use the dipole vector directly in moldscript/fmo.py:26. moments[0] is the multipole reference point and moments[1] is the dipole vector, so subtracting them changes the reported dipole whenever the origin is nonzero. np.linalg.norm(fmo_data.moments[1]) is the right value.
🤖 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 `@moldscript/fmo.py` around lines 25 - 26, Update the dipole calculation in the
fmo.py flow to compute the norm directly from fmo_data.moments[1] using
np.linalg.norm, removing the subtraction from fmo_data.moments[0] while
preserving the reported scalar dipole value.
| softness = lumo - homo | ||
| chemical_potential = (lumo + homo) / 2 | ||
| global_electrophilicity = chemical_potential**2 / (2 * softness) | ||
| mol_values = { | ||
| "dipole": dipole, | ||
| "HOMO": homo, | ||
| "LUMO": lumo, | ||
| "HOMO-LUMO_gap": softness, | ||
| "chemical_potential": chemical_potential, | ||
| "global_electrophilicity": global_electrophilicity, | ||
| "global_nucleophilicity": 1 / global_electrophilicity, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Correct the electrophilicity denominator.
Under the frontier-orbital approximation, hardness is half the HOMO–LUMO gap and electrophilicity is μ²/(2η). Using the full gap as softness and multiplying it by two halves every reported value. (pubs.rsc.org)
Proposed fix
- softness = lumo - homo
+ gap = lumo - homo
+ hardness = gap / 2
chemical_potential = (lumo + homo) / 2
- global_electrophilicity = chemical_potential**2 / (2 * softness)
+ global_electrophilicity = chemical_potential**2 / (2 * hardness)🤖 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 `@moldscript/fmo.py` around lines 29 - 39, Update the electrophilicity
calculation in the mol_values computation so the denominator uses hardness,
defined as half of the HOMO–LUMO gap, while preserving the existing
chemical_potential and reported gap values. Ensure global_electrophilicity is
calculated as μ²/(2η) and global_nucleophilicity continues to use its
reciprocal.
| for i, line in enumerate(lines): | ||
| if line.find(" Summary of Natural Population Analysis:") > -1: | ||
| list_npop.append(i + 6) | ||
| if list_npop: | ||
| start_npop = list_npop[0] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use the final NPA table.
Optimization outputs may contain multiple NPA sections. Selecting list_npop[0] combines first-geometry charges with scfenergies[-1], producing internally inconsistent Fukui descriptors.
- start_npop = list_npop[0]
+ start_npop = list_npop[-1]📝 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.
| for i, line in enumerate(lines): | |
| if line.find(" Summary of Natural Population Analysis:") > -1: | |
| list_npop.append(i + 6) | |
| if list_npop: | |
| start_npop = list_npop[0] | |
| for i, line in enumerate(lines): | |
| if line.find(" Summary of Natural Population Analysis:") > -1: | |
| list_npop.append(i + 6) | |
| if list_npop: | |
| start_npop = list_npop[-1] |
🤖 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 `@moldscript/fukui.py` around lines 31 - 35, Update the NPA section selection
in the loop that populates list_npop so start_npop uses the final detected NPA
table, matching scfenergies[-1], rather than list_npop[0]. Preserve the existing
behavior when no NPA section is found.
| if args.volume != False or args.vall != False: | ||
| data_dicts = sterics(first_read, data_dicts, args.volume, args.vall, args.radius, output=args.output).dd |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Handle missing base inputs for sterics, resolve NameError, and avoid boolean anti-patterns.
This code segment contains three issues:
- Comparing directly to
Falseis an anti-pattern. Use truthy checks (args.volume or args.vall) instead. - If
args.linkis passed, theelseblock is skipped andfirst_readis never defined, leading to aNameErrorhere. - If sterics are requested without a base input (e.g., using only
--mlip),first_readevaluates to''and causes anAttributeErrorinsidestericsbecause it expects a dictionary.
Add a fallback to initialize first_read (e.g., to None) at the top of the function and guard this block so it only runs if first_read contains valid structural files.
At the top of main() (around line 68), ensure first_read is initialized unconditionally:
data_dicts = {}
+ first_read = None🐛 Proposed fix for the sterics block
- if args.volume != False or args.vall != False:
- data_dicts = sterics(first_read, data_dicts, args.volume, args.vall, args.radius, output=args.output).dd
+ if args.volume or args.vall:
+ if not first_read:
+ terminal_error("x Sterics calculation requires a base input (e.g., --opt) to provide geometry files.")
+ sys.exit(1)
+ data_dicts = sterics(first_read, data_dicts, args.volume, args.vall, args.radius, output=args.output).dd🧰 Tools
🪛 Ruff (0.15.21)
[error] 160-160: Avoid inequality comparisons to False; use args.volume: for truth checks
Replace with args.volume
(E712)
[error] 160-160: Avoid inequality comparisons to False; use args.vall: for truth checks
Replace with args.vall
(E712)
🤖 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 `@moldscript/moldscript.py` around lines 160 - 161, Update main() to initialize
first_read to None alongside data_dicts before the input-selection branches,
preventing the --link path from raising NameError. Replace the direct False
comparisons in the sterics condition with a truthy check for args.volume or
args.vall, and only invoke sterics when those options are requested and
first_read contains valid structural input rather than None or an empty value.
| def initialize_run_log(output_prefix, version, timestamp, reference, argv=None): | ||
| """Create the single run-level .dat file and write run provenance.""" | ||
| logger = Logger(build_log_path(output_prefix), verbose=False) | ||
| logger.write_only( | ||
| f" MOLDSCRIPT v {version} {timestamp} \n Citation: {reference}\n" | ||
| ) | ||
| command_line = " ".join(["python", "-m", "moldscript", *(argv or [])]) | ||
| logger.write_only(f"Command line used in MOLDSCRIPT: {command_line}") | ||
| logger.finalize() |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Explicitly truncate the audit log when initializing a new run.
A second main() invocation using the same output path in one process appends to the previous run because _LOG_PATHS_INITIALIZED is never reset. This mixes independent run provenance.
Proposed fix
def initialize_run_log(output_prefix, version, timestamp, reference, argv=None):
- logger = Logger(build_log_path(output_prefix), verbose=False)
+ logger = Logger(build_log_path(output_prefix), verbose=False, truncate=True)
class Logger:
- def __init__(self, file_path=None, verbose=True):
+ def __init__(self, file_path=None, verbose=True, truncate=False):
...
- self.started_new_file = key not in _LOG_PATHS_INITIALIZED
+ self.started_new_file = truncate or key not in _LOG_PATHS_INITIALIZEDAlso applies to: 282-295
🤖 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 `@moldscript/utils.py` around lines 267 - 275, Update initialize_run_log and
the related _LOG_PATHS_INITIALIZED handling so each new run explicitly truncates
the audit log and clears any prior path-initialization state before writing
provenance. Ensure repeated main() invocations with the same output path start
with an independent log while preserving the existing run metadata and
command-line entries.
| def resolve_data_key(fullname, data_dict, module_name=None, logger=None): | ||
| """Resolve a module filename/key to an initialized molecule key.""" | ||
| available = set(molecule_keys(data_dict)) | ||
| for candidate in filename_match_candidates(fullname): | ||
| if candidate in available: | ||
| return candidate |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not resolve calculator files by unrestricted prefix truncation.
For example, foo_bar_spc.log falls back to molecule foo when foo_bar is absent, silently writing descriptors to the wrong record. Strip only the configured module suffix, or reject matches requiring removal of molecule-name components.
🤖 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 `@moldscript/utils.py` around lines 519 - 524, Update resolve_data_key and
filename_match_candidates so calculator filenames are not matched by
unrestricted prefix truncation. Only strip the configured module suffix when
deriving candidates, or reject candidates that remove molecule-name components;
ensure names such as foo_bar_spc.log do not resolve to foo when foo_bar is
unavailable.
| def bond_data_matrix(data): | ||
| try: | ||
| coords = np.asarray(data.atomcoords[-1], dtype=float) | ||
| except: | ||
| coords = np.asarray(data, dtype=float) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant section with line numbers
sed -n '610,670p' moldscript/utils.py | cat -n
# Find call sites and nearby handling to understand intended inputs
rg -n "bond_data_matrix|atomcoords" -S .Repository: patonlab/molDscript
Length of output: 3673
Catch only the expected conversion errors here.
The bare except also swallows KeyboardInterrupt and SystemExit; limit it to the attribute/index/type/value failures that can happen during coordinate extraction.
Proposed fix
- except:
+ except (AttributeError, IndexError, TypeError, ValueError):
coords = np.asarray(data, dtype=float)📝 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 bond_data_matrix(data): | |
| try: | |
| coords = np.asarray(data.atomcoords[-1], dtype=float) | |
| except: | |
| coords = np.asarray(data, dtype=float) | |
| def bond_data_matrix(data): | |
| try: | |
| coords = np.asarray(data.atomcoords[-1], dtype=float) | |
| except (AttributeError, IndexError, TypeError, ValueError): | |
| coords = np.asarray(data, dtype=float) |
🧰 Tools
🪛 Ruff (0.15.21)
[error] 638-638: Do not use bare except
(E722)
🤖 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 `@moldscript/utils.py` around lines 635 - 639, Update bond_data_matrix so its
coordinate-extraction fallback catches only expected attribute, indexing, and
conversion failures (such as AttributeError, IndexError, TypeError, and
ValueError), rather than using a bare except that also intercepts
process-control exceptions.
Source: Linters/SAST tools
| - `--output PREFIX` - prepend every generated filename; append a slash to target a directory. Use `--no_mol`, `--no_atom`, `--no_bond`, or `--no_bond_filter` to tailor CSV output. | ||
| - `--substructure SMARTS` - limit atom/bond descriptors to a SMARTS match; combine with `--volume` or `--vall` and optional `--radius` list for DBSTEP buried volumes. | ||
| - `--boltz`, `--min_max`, `--lowe` - compute Boltzmann-weighted averages, min/max/range tables (using `--cut`), and lowest-energy snapshots. Adjust `--temp` (K) as needed. | ||
| - `--output PREFIX` - prepend every generated filename; append a slash to target a directory. Use `--no_mol`, `--no_atom`, `--no_bond`, or `--no_bond_filter` to tailor CSV output. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Document --no_bond_filter separately from CSV toggles.
--no_bond_filter changes bond inclusion criteria; it does not disable CSV output. Keep it separate from --no_mol, --no_atom, and --no_bond, consistent with docs/source/README.rst Lines 165-169.
🤖 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 `@README.md` at line 43, Update the README option description near the output
PREFIX entry to list --no_mol, --no_atom, and --no_bond as CSV-output toggles,
while documenting --no_bond_filter separately as the bond-inclusion control;
keep the wording consistent with docs/source/README.rst.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
moldscript/argument_parser.py (1)
96-102: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winImprove numeric parsing to support negative integers.
The
.isdigit()check returnsFalsefor negative integers (e.g.,"-4"), causing them to fall through to thefloatparsing block and become floats instead of integers. Using atry/exceptchain provides more robust parsing for all numeric types.🐛 Proposed fix
- elif value.isdigit(): - value = int(value) - else: - try: - value = float(value) - except ValueError: - pass # Keep value as a string if it can't be converted + else: + try: + value = int(value) + except ValueError: + try: + value = float(value) + except ValueError: + pass # Keep value as a string if it can't be converted🤖 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 `@moldscript/argument_parser.py` around lines 96 - 102, Update the numeric conversion branch in the argument parsing logic to attempt integer conversion before float conversion, using exception handling so negative integer strings such as “-4” remain integers while decimal values become floats and non-numeric values remain strings.
🤖 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.
Outside diff comments:
In `@moldscript/argument_parser.py`:
- Around line 96-102: Update the numeric conversion branch in the argument
parsing logic to attempt integer conversion before float conversion, using
exception handling so negative integer strings such as “-4” remain integers
while decimal values become floats and non-numeric values remain strings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a987a635-0608-4749-b4df-ac595cd4fca6
📒 Files selected for processing (3)
README.mdmoldscript/argument_parser.pymoldscript/moldscript.py
🚧 Files skipped from review as they are similar to previous changes (1)
- moldscript/moldscript.py
1 participant
adding mlip support and spin density
Summary by CodeRabbit
New Features
.extxyz) processing for neutral, reduced, and oxidized datasets, generating molecule-, atom-, and bond-level outputs.--workersparallel parsing support and optional argument-file writing.Bug Fixes
Documentation
MOLDSCRIPT.dat.Tests