Delegate parsing and scaling factors to GoodVibes; add ORCA support#3
Open
bobbypaton wants to merge 18 commits into
Open
Delegate parsing and scaling factors to GoodVibes; add ORCA support#3bobbypaton wants to merge 18 commits into
bobbypaton wants to merge 18 commits into
Conversation
Kinisot's core is the Bigeleisen-Mayer math; file parsing now comes from GoodVibes (>= 4.4, new dependency): - read_hess() mass-weights the Cartesian Hessian returned by goodvibes.io.parse_hessian (Gaussian archive block, or ORCA .hess found next to the .out / passed directly). - Isotope substitution matches elements by mass window so Gaussian's isotopic masses (12.000) and ORCA's average masses (12.011) both work. - level_of_theory and linearity (point-group based) come from GoodVibes' program-agnostic parsers. - The vendored Truhlar v3b2 table is deleted in favor of the v5 database bundled with GoodVibes (canonicalize_level handles cross-program functional naming; R/U/RO spin prefixes still tolerated). All 9 Gaussian golden values are unchanged. New ORCA end-to-end tests: n-pentane TT/GG conformer EQE with D substitution (examples/orca/), including the .hess-direct invocation path. Requires the GoodVibes feat/hessian-parsing branch (unreleased); CI on this branch will be red until GoodVibes 4.4 is on PyPI.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR moves parsing, isotope substitution, scaling lookup, thermodynamics, and CLI execution onto GoodVibes-based code paths, adds ORCA support, updates packaging/versioning, and expands tests and CI coverage. ChangesGoodVibes delegation and ORCA support
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (1)
kinisot/Hess_to_Freq.py (1)
27-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSplit single-line
ifstatements (Ruff E701).Static analysis flags multiple statements per line for all three isotope-window checks.
🔧 Proposed fix
- if 0.9 < mass_list[i] < 1.5: mass_list[i] = 2.0141 # 1H - 2D - if 11.5 < mass_list[i] < 12.5: mass_list[i] = 13.00335 # 12C - 13C - if 15.5 < mass_list[i] < 16.5: mass_list[i] = 16.9991 # 16O - 17O + if 0.9 < mass_list[i] < 1.5: + mass_list[i] = 2.0141 # 1H - 2D + if 11.5 < mass_list[i] < 12.5: + mass_list[i] = 13.00335 # 12C - 13C + if 15.5 < mass_list[i] < 16.5: + mass_list[i] = 16.9991 # 16O - 17O🤖 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 `@kinisot/Hess_to_Freq.py` around lines 27 - 29, The isotope-window checks in the mass remapping block use single-line `if` statements, which triggers Ruff E701. In `Hess_to_Freq.py`, split each conditional assignment for the `mass_list[i]` normalization logic into its own multi-line `if` block so each `if` and assignment are on separate lines, keeping the same thresholds and replacement masses.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 `@kinisot/Hess_to_Freq.py`:
- Around line 22-30: The substitute_isotopes logic in Hess_to_Freq.py silently
ignores invalid or unsupported isotope targets, so add an explicit guard in that
function to fail or warn when an atom index is out of range or when the selected
mass does not match any supported isotope window. Update the loop that parses
iso and adjusts mass_list so that every requested substitution is validated and
unexpected masses do not pass through unchanged without notice.
In `@setup.py`:
- Around line 23-24: The dependency constraint in setup.py is too strict because
goodvibes>=4.4 is not yet available on PyPI, so fresh installs will fail. Update
the install_requires entry for goodvibes to an available released version, or
temporarily relax/gate the version bump in setup.py so installation can proceed
until the upstream release exists.
---
Nitpick comments:
In `@kinisot/Hess_to_Freq.py`:
- Around line 27-29: The isotope-window checks in the mass remapping block use
single-line `if` statements, which triggers Ruff E701. In `Hess_to_Freq.py`,
split each conditional assignment for the `mass_list[i]` normalization logic
into its own multi-line `if` block so each `if` and assignment are on separate
lines, keeping the same thresholds and replacement masses.
🪄 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: 3e529af9-c8a2-4e15-9b44-f45d6f0fa51b
⛔ Files ignored due to path filters (2)
kinisot/examples/orca/pentane_GG.outis excluded by!**/*.outkinisot/examples/orca/pentane_TT.outis excluded by!**/*.out
📒 Files selected for processing (9)
CHANGELOG.mdREADME.mdkinisot/Hess_to_Freq.pykinisot/Kinisot.pykinisot/examples/orca/pentane_GG.hesskinisot/examples/orca/pentane_TT.hesskinisot/vib_scale_factors.pysetup.pytests/test_kinisot.py
💤 Files with no reviewable changes (1)
- kinisot/vib_scale_factors.py
Library:
- Explicit imports replace the bare-except shim and 'import *'; public
API re-exported from kinisot/__init__.py with a single-source
__version__ (2.1.0).
- Logger is a context manager; Fatal() writes the message to the log
and closes it before exiting. main() can no longer leak the handle.
- calc_zpe_factor uses the log-domain identity 0.5*hv/kT directly
instead of np.log(math.exp(...)), avoiding exp overflow at very low
temperatures. Dead rounded-frequency code removed.
- warnings.warn when a structure has >1 imaginary frequency above the
cutoff (higher-order saddle point).
- substitute_isotopes validates atom numbers: out-of-range or
non-numeric labels raise ValueError with the offending file ('0'
still means no substitution).
- compute_isotope_effect raises if called with neither ts nor prd.
CLI:
- Argument validation happens before the output file is created, via
parser.error: missing or conflicting --ts/--prd, label/file count
mismatch. Unknown flags are now rejected (parse_args, not
parse_known_args).
- -s uses None as the auto-detect sentinel instead of False.
- --version flag; --iso/--rct help documents the '0' convention.
- Level-of-theory consistency check covers all input files, not just
the first two.
Covers: out-of-range/non-numeric --iso labels, missing ts/prd in the API, isotope mass windows (Gaussian vs ORCA conventions), and the CLI via subprocess: KIE (Gaussian), EQE (ORCA), --version, all four argument-error cases (exit 2, no stray output file), and the no-imaginary-TS failure (exit 1).
…(Phase 3) - pyproject.toml (PEP 621) replaces setup.py/setup.cfg; version is single-sourced from kinisot.__version__; 'kinisot' console script. - Examples move from inside the package to examples/ at the repo root: the wheel shrinks from several MB of Gaussian outputs to ~12 kB. conftest resolves test data from the checkout instead of the installed package. - CI: new ruff lint job (check + format --check) and pytest coverage. - .gitignore covers build/, __pycache__/, pytest/coverage artifacts. - README documents the console command, EQE mode, ORCA inputs, and the repeat-per-file --iso convention.
One-time reformat: 4-space indentation replaces the mixed 3-space style, one statement per line, normalized quotes/spacing. No behavioral changes; the full test suite is identical before and after.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
10-22: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueOptional CI hardening: scope token permissions and disable credential persistence.
The new
lintjob runs with default (broad)GITHUB_TOKENpermissions and persists checkout credentials, though it only reads code to run ruff.🔒 Suggested hardening
lint: runs-on: ubuntu-latest permissions: contents: read steps: - uses: actions/checkout@v4 with: persist-credentials: false🤖 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 @.github/workflows/ci.yml around lines 10 - 22, The lint job currently uses default broad GITHUB_TOKEN permissions and leaves checkout credentials persisted, which is unnecessary for ruff-only validation. Update the lint job in the workflow to explicitly set permissions to contents: read and configure the actions/checkout@v4 step with persist-credentials: false. Keep the change scoped to the lint job and preserve the existing ruff check and ruff format --check steps.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 `@README.md`:
- Line 45: Fix the spelling in the README temperature description by correcting
“evalulated” to “evaluated” in the `-t` option text; update the wording in that
sentence only and keep the rest of the `-t` documentation unchanged.
- Line 46: Update the `-s` option description in the README to mention the new
fallback behavior: auto-detection via GoodVibes still applies when a level of
theory and basis set match, but renamed or non-matching levels may now require
`-s` to be set explicitly. Adjust the wording in the `-s` documentation section
so it does not imply all cases are automatically detected, and clarify that the
default remains 1 when no scaling factor is available.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 10-22: The lint job currently uses default broad GITHUB_TOKEN
permissions and leaves checkout credentials persisted, which is unnecessary for
ruff-only validation. Update the lint job in the workflow to explicitly set
permissions to contents: read and configure the actions/checkout@v4 step with
persist-credentials: false. Keep the change scoped to the lint job and preserve
the existing ruff check and ruff format --check steps.
🪄 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: 8e083abc-cc18-4013-81f3-b9dc0cd59204
⛔ Files ignored due to path filters (15)
examples/gaussian/DATS.outis excluded by!**/*.outexamples/gaussian/DATS_rct.outis excluded by!**/*.outexamples/gaussian/Kinisot_output.datis excluded by!**/*.datexamples/gaussian/claisen_gs.outis excluded by!**/*.outexamples/gaussian/claisen_kinisot.datis excluded by!**/*.datexamples/gaussian/claisen_prd.outis excluded by!**/*.outexamples/gaussian/claisen_ts.outis excluded by!**/*.outexamples/gaussian/comparison.datis excluded by!**/*.datexamples/gaussian/diels_alder_kinisot.datis excluded by!**/*.datexamples/gaussian/diene.outis excluded by!**/*.outexamples/gaussian/dienophile.outis excluded by!**/*.outexamples/gaussian/tetramethylcyclohexane.outis excluded by!**/*.outexamples/gaussian/tetramethylcyclohexane_kinisot.datis excluded by!**/*.datexamples/orca/pentane_GG.outis excluded by!**/*.outexamples/orca/pentane_TT.outis excluded by!**/*.out
📒 Files selected for processing (22)
.github/workflows/ci.yml.gitignoreCHANGELOG.mdIMPLEMENTATION_PLAN.mdMANIFEST.inREADME.mdexamples/gaussian/claisen_kinisot.shexamples/gaussian/comparison.shexamples/gaussian/diels_alder_kinisot.shexamples/gaussian/tetramethylcyclohexane_kinisot.shexamples/orca/pentane_GG.hessexamples/orca/pentane_TT.hesskinisot/Hess_to_Freq.pykinisot/Kinisot.pykinisot/__init__.pykinisot/__main__.pypyproject.tomlsetup.cfgsetup.pytests/conftest.pytests/test_cli.pytests/test_kinisot.py
💤 Files with no reviewable changes (2)
- setup.cfg
- setup.py
✅ Files skipped from review due to trivial changes (5)
- .gitignore
- IMPLEMENTATION_PLAN.md
- pyproject.toml
- kinisot/main.py
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_kinisot.py
| * The two output files contain frequency calculations performed for the reactant and transition state at the same level of theory. Gaussian output files and ORCA outputs (with the `.hess` file next to the `.out`, or the `.hess` passed directly) are supported. | ||
| * For an equilibrium isotope effect, pass `--prd product_output` instead of `--ts`. | ||
| * The `--iso` flag is required and specifies comma-separated atom number(s) which are to be substituted for heavier isotopes. With several input files, repeat the flag once per file (`--iso 0` for a file with no substitution); a single `--iso` is applied to reactant and TS/product alike. | ||
| * The `-t` option specifies temperature (in Kelvin). N.B. This does not have to correspond to the temperature used in the underlying calculation since the Reduced Isotopic Partition Function Ratios are evalulated at the requested temperature. The default value is 298.15 K. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the spelling here.
“evalulated” should be “evaluated”.
🧰 Tools
🪛 LanguageTool
[grammar] ~45-~45: Ensure spelling is correct
Context: ... Isotopic Partition Function Ratios are evalulated at the requested temperature. The defau...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_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 `@README.md` at line 45, Fix the spelling in the README temperature description
by correcting “evalulated” to “evaluated” in the `-t` option text; update the
wording in that sentence only and keep the rest of the `-t` documentation
unchanged.
Source: Linters/SAST tools
| * For an equilibrium isotope effect, pass `--prd product_output` instead of `--ts`. | ||
| * The `--iso` flag is required and specifies comma-separated atom number(s) which are to be substituted for heavier isotopes. With several input files, repeat the flag once per file (`--iso 0` for a file with no substitution); a single `--iso` is applied to reactant and TS/product alike. | ||
| * The `-t` option specifies temperature (in Kelvin). N.B. This does not have to correspond to the temperature used in the underlying calculation since the Reduced Isotopic Partition Function Ratios are evalulated at the requested temperature. The default value is 298.15 K. | ||
| * The `-s` option is a scaling factor for vibrational frequencies. Empirical ZPE-scaling factors from the [Truhlar group database](https://comp.chem.umn.edu/freqscale/) are applied automatically (via GoodVibes) based on detection of the level of theory and basis set in the output files. The default value when no scaling factor is available is 1 (no scale factor). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Mention the new -s fallback behavior.
This says scaling factors are applied automatically via GoodVibes, but the PR also changes matching so some renamed levels no longer auto-match unless -s is set explicitly. Without that caveat, the README overpromises auto-detection.
Suggested wording
-* The `-s` option is a scaling factor for vibrational frequencies. Empirical ZPE-scaling factors from the [Truhlar group database](https://comp.chem.umn.edu/freqscale/) are applied automatically (via GoodVibes) based on detection of the level of theory and basis set in the output files. The default value when no scaling factor is available is 1 (no scale factor).
+* The `-s` option is a scaling factor for vibrational frequencies. Empirical ZPE-scaling factors from the [Truhlar group database](https://comp.chem.umn.edu/freqscale/) are applied automatically (via GoodVibes) based on detection of the level of theory and basis set in the output files. Some renamed levels may require `-s` to be set explicitly. The default value when no scaling factor is available is 1 (no scale factor).📝 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.
Suggested change
| * The `-s` option is a scaling factor for vibrational frequencies. Empirical ZPE-scaling factors from the [Truhlar group database](https://comp.chem.umn.edu/freqscale/) are applied automatically (via GoodVibes) based on detection of the level of theory and basis set in the output files. The default value when no scaling factor is available is 1 (no scale factor). | |
| * The `-s` option is a scaling factor for vibrational frequencies. Empirical ZPE-scaling factors from the [Truhlar group database](https://comp.chem.umn.edu/freqscale/) are applied automatically (via GoodVibes) based on detection of the level of theory and basis set in the output files. Some renamed levels may require `-s` to be set explicitly. The default value when no scaling factor is available is 1 (no scale factor). |
🤖 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 46, Update the `-s` option description in the README to
mention the new fallback behavior: auto-detection via GoodVibes still applies
when a level of theory and basis set match, but renamed or non-matching levels
may now require `-s` to be set explicitly. Adjust the wording in the `-s`
documentation section so it does not imply all cases are automatically detected,
and clarify that the default remains 1 when no scaling factor is available.
The Bigeleisen-Mayer equation compares isotopically pure species, so the light isotopologue must be mass-weighted with principal isotope masses (1H 1.00783, 12C 12.00000, 16O 15.99491). Gaussian prints exactly these, but ORCA .hess files store abundance-averaged atomic masses (C = 12.011), which biased ORCA-derived KIEs at the ~1e-4 level. read_hess now maps the substitutable elements onto principal masses before substitution; other elements keep the program's value, which cancels almost exactly in the RPFR ratios. Gaussian golden values are bit-identical; the ORCA pentane EQE golden moves from 1.007728 to 1.007730. Also adds Phase 5 plan items: multi-isotopologue runs, referenced (relative) KIEs, Wigner tunneling, and a primary-H tunneling caveat warning.
…topologue runs (Phases 4-5)
Internal refactor (numerics unchanged, goldens identical):
- kinisot/thermo.py: constants and the Bigeleisen-Mayer math. RPFR and
frozen IsotopeEffect dataclasses replace the class-used-as-function
and the 8-tuple; factor functions are vectorized over frequency
arrays (np.expm1 in the excitation term).
- kinisot/scaling.py: Truhlar-database lookup, logger-free.
- kinisot/cli.py: argument parsing and all .dat/terminal formatting.
- kinisot/isotopes.py: isotope mass table.
- kinisot.Kinisot remains as a deprecation shim preserving the historic
tuple/class API (DeprecationWarning), to be removed in a future
release.
New features:
- Isotope table: 2D, 3T, 13C, 14C, 15N, 17O, 18O with an atom:isotope
--iso syntax ('5:18O,7:2D'); plain atom numbers keep their default
heavy isotope (now including 15N for nitrogen). Substituting an
element with no isotope data is now an error instead of a silent
no-op.
- Multiple isotopologues per invocation ('--iso C5=5;C4=4;HD=7,8'),
computed with shared light-species RPFRs (N+1 instead of 2N
diagonalizations per species).
- Referenced (relative) isotope effects via --ref NAME, matching the
internal-standard convention of experimental KIE measurements.
- Wigner tunneling correction reported alongside Bell in the output
table and IsotopeEffect.
- --output PATH, --quiet, and --json PATH; a warning flags large KIEs
where 1-D tunneling corrections are least reliable.
…Phase 6) - README documents the isotope-selection syntax, multi-isotopologue runs, --ref, output options, and the structured Python API. - CITATION.cff with the Zenodo concept DOI (verified via the badge redirect). - Tag-driven PyPI publishing via trusted publishing (needs the 'pypi' environment configured on GitHub + a PyPI trusted publisher for the repo before first use); Dependabot for Actions versions. - CHANGELOG and plan status updated.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
.github/workflows/publish.yml (2)
1-23: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd least-privilege
permissionsblocks.Neither the workflow nor the
buildjob declarespermissions, so both default to the broad implicitGITHUB_TOKENscope. Thepublishjob already scopes its own permissions;build(which only checks out code and builds artifacts) should be pinned down too.🔒️ Proposed fix
name: Publish to PyPI +permissions: + contents: read + on: release: types: [published] workflow_dispatch: jobs: build: runs-on: ubuntu-latest + permissions: + contents: read steps: - uses: actions/checkout@v4🤖 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 @.github/workflows/publish.yml around lines 1 - 23, The workflow lacks explicit least-privilege GitHub token scoping for the build path, so add a restrictive permissions block for the overall workflow or at least the build job. In the build job that uses actions/checkout, actions/setup-python, python -m build, and actions/upload-artifact, set permissions to the minimum needed for checkout and artifact upload, and leave the publish job’s tighter permissions intact.Source: Linters/SAST tools
12-12: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueSet
persist-credentials: falseon checkout.The
buildjob's checkout doesn't disable credential persistence, so the ephemeralGITHUB_TOKENremains in the local git config for the rest of the job (used only for building/uploading an artifact here, no push needed).🔒️ Proposed fix
- uses: actions/checkout@v4 + with: + persist-credentials: false🤖 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 @.github/workflows/publish.yml at line 12, The checkout step in the publish workflow still persists the job token in git config; update the existing actions/checkout@v4 usage in the build job to set persist-credentials to false so the GITHUB_TOKEN is not kept for the rest of the job. Keep the change localized to the checkout step in the publish workflow since no git push is needed in this job.Source: Linters/SAST tools
kinisot/thermo.py (2)
73-110: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRedundant per-file re-parsing across isotopologues.
read_hess()(parse_hessian) andis_linear()(parse_qcdata) each independently parse the same file, andrpfr()is invoked once for the light species plus once per isotopologue incompute_isotope_effects. Since linearity is isotope-independent,is_linear(file)re-parses the same output file N+1 times for the same result. For large Gaussian/ORCA outputs (or many isotopologues via--iso ... ; ...), this multiplies file I/O unnecessarily.Consider memoizing
is_linear(and/or the raw parsed Hessian/qcdata) keyed by file path, e.g. withfunctools.lru_cache, inHess_to_Freq.py.🤖 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 `@kinisot/thermo.py` around lines 73 - 110, The rpfr() flow is repeatedly re-parsing the same output files because is_linear(file) and read_hess(file, isomer[i]) both do file-based parsing, and compute_isotope_effects calls rpfr once per isotopologue. Memoize the file-independent parsing in Hess_to_Freq.py, especially is_linear() (and optionally the parsed Hessian/qcdata behind read_hess()), using a cache keyed by file path so repeated rpfr() calls reuse results instead of re-reading the same file.
199-246: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winValidate label length before slicing.
compute_isotope_effectsis a public API (re-exported fromkinisot.__init__), but it doesn't validate that eachlabelinlabelshaslen(rct) + len(other)entries before slicing (label[0:len(rct)],label[len(rct):]). A caller invoking this directly (bypassingcli.py's own length check) with a mismatched label gets a downstreamIndexErrorinsiderpfr()'sisomer[i]access rather than a clear error at the entry point.🛡️ Proposed fix
if ts is not None: other, is_kie = ts, True elif prd is not None: other, is_kie = prd, False else: raise ValueError( "compute_isotope_effect needs a TS (KIE) or a product (EQE) in addition to the reactant" ) + expected_len = len(rct) + len(other) + for label in labels: + if len(label) != expected_len: + raise ValueError( + "each label must have {} entries (one per rct/ts/prd file), got {}".format( + expected_len, len(label) + ) + ) + light_rct = rpfr(rct, ["0"] * len(rct), temperature, freq_scale_factor, freq_cutoff)🤖 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 `@kinisot/thermo.py` around lines 199 - 246, In compute_isotope_effects, add upfront validation that every label has exactly len(rct) + len(other) entries before slicing it into reactant and other parts. If a label length is invalid, raise a clear ValueError at the start of the function instead of letting rpfr() fail later with an IndexError. Use compute_isotope_effects and its label[0:len(rct)] / label[len(rct):] splitting logic as the place to enforce this check.kinisot/cli.py (1)
357-369: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNarrow exception filter for the compute call.
Only
ValueError/FileNotFoundErrorproduce the friendlylog.Fatalmessage; otherOSErrorsubtypes (e.g.IsADirectoryError,PermissionError) from file access insidecompute_isotope_effectswould instead surface as a raw traceback.♻️ Proposed fix
- except (ValueError, FileNotFoundError) as e: + except (ValueError, OSError) as e: log.Fatal("\no " + str(e))🤖 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 `@kinisot/cli.py` around lines 357 - 369, The exception handling around compute_isotope_effects only covers ValueError and FileNotFoundError, so other file-related OSError subclasses can still leak a traceback. Update the try/except in the CLI flow that calls compute_isotope_effects to also handle OSError (or the relevant broader file access exception family) and route it through the same log.Fatal path. Keep the existing friendly message behavior for the compute_isotope_effects call and preserve the current error formatting in kinisot/cli.py.
🤖 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 `@kinisot/cli.py`:
- Around line 93-94: The _stem() helper is splitting on every dot in the full
path, which truncates directory names and can produce wrong or empty species
labels in the .dat report. Update _stem() in cli.py to derive the stem from just
the filename portion rather than the whole path, using the existing _stem() call
sites to keep report labels correct for paths like ../data/mol.out and
run.v2/mol.out.
---
Nitpick comments:
In @.github/workflows/publish.yml:
- Around line 1-23: The workflow lacks explicit least-privilege GitHub token
scoping for the build path, so add a restrictive permissions block for the
overall workflow or at least the build job. In the build job that uses
actions/checkout, actions/setup-python, python -m build, and
actions/upload-artifact, set permissions to the minimum needed for checkout and
artifact upload, and leave the publish job’s tighter permissions intact.
- Line 12: The checkout step in the publish workflow still persists the job
token in git config; update the existing actions/checkout@v4 usage in the build
job to set persist-credentials to false so the GITHUB_TOKEN is not kept for the
rest of the job. Keep the change localized to the checkout step in the publish
workflow since no git push is needed in this job.
In `@kinisot/cli.py`:
- Around line 357-369: The exception handling around compute_isotope_effects
only covers ValueError and FileNotFoundError, so other file-related OSError
subclasses can still leak a traceback. Update the try/except in the CLI flow
that calls compute_isotope_effects to also handle OSError (or the relevant
broader file access exception family) and route it through the same log.Fatal
path. Keep the existing friendly message behavior for the
compute_isotope_effects call and preserve the current error formatting in
kinisot/cli.py.
In `@kinisot/thermo.py`:
- Around line 73-110: The rpfr() flow is repeatedly re-parsing the same output
files because is_linear(file) and read_hess(file, isomer[i]) both do file-based
parsing, and compute_isotope_effects calls rpfr once per isotopologue. Memoize
the file-independent parsing in Hess_to_Freq.py, especially is_linear() (and
optionally the parsed Hessian/qcdata behind read_hess()), using a cache keyed by
file path so repeated rpfr() calls reuse results instead of re-reading the same
file.
- Around line 199-246: In compute_isotope_effects, add upfront validation that
every label has exactly len(rct) + len(other) entries before slicing it into
reactant and other parts. If a label length is invalid, raise a clear ValueError
at the start of the function instead of letting rpfr() fail later with an
IndexError. Use compute_isotope_effects and its label[0:len(rct)] /
label[len(rct):] splitting logic as the place to enforce this check.
🪄 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: 3fffdb44-215c-4f57-85ba-5379d620d3ea
📒 Files selected for processing (16)
.github/dependabot.yml.github/workflows/publish.ymlCHANGELOG.mdCITATION.cffIMPLEMENTATION_PLAN.mdREADME.mdkinisot/Hess_to_Freq.pykinisot/Kinisot.pykinisot/__init__.pykinisot/__main__.pykinisot/cli.pykinisot/isotopes.pykinisot/scaling.pykinisot/thermo.pypyproject.tomltests/test_kinisot.py
✅ Files skipped from review due to trivial changes (3)
- .github/dependabot.yml
- CITATION.cff
- IMPLEMENTATION_PLAN.md
🚧 Files skipped from review as they are similar to previous changes (4)
- kinisot/main.py
- pyproject.toml
- kinisot/Hess_to_Freq.py
- tests/test_kinisot.py
Covers the GoodVibes canonicalization fix: Pople star shorthand (6-31+G** == 6-31+G(d,p), 6-31G* == 6-31G(d)) and ORCA hyphenated basis names (def2-TZVP == def2TZVP) now resolve to database entries.
…sover T New examples/orca/ HAT case (ORCA 6.1, broken-symmetry M06-2X-D3/6-31+G** with SMD): true-minimum ground state plus an H-transfer TS with a -1974 cm-1 imaginary mode. Golden tests pin the primary (3.189) and secondary (1.037) H/D KIEs and the 3T KIE (5.115, correctly larger than D), plus end-to-end ORCA scaling-factor auto-detection (0.968 via the 6-31+G** == 6-31+G(d,p) normalization). The Freq-only .inp files used to regenerate the .hess data are included. The case exposed that the Bell infinite-parabola correction was silently meaningless below the crossover temperature T_c = h*nu*c/2*pi*k (u/2 > pi, where the sine ratio loses its meaning -- here it returned 0.64, *reducing* a primary H-transfer KIE at 298 K with T_c ~ 440 K). Kinisot now warns and reports Bell values as nan in that regime; the uncorrected and Wigner values remain available.
The hand-ljust()ed .dat layout misaligned whenever file paths were long and squeezed 9 numeric columns onto unlabeled rows. Output (terminal and .dat alike) is now rendered by Rich: a species table (with the TS imaginary frequency), the main isotope-effect table, an RPFR components table, and the referenced-KIE table when --ref is used. EQE runs drop the meaningless tunneling columns entirely. Library warnings raised during computation are captured and rendered once, deduplicated, instead of leaking as raw Python warnings; nan (Bell below crossover) renders as an em dash. rich>=13 becomes an explicit dependency (it was already in the tree via GoodVibes). The legacy plain Logger remains for the kinisot.Kinisot shim.
Unnamed isotopologues with the same substitution in every file were labeled '26 / 26', which reads like a fraction; identical (and '0') specs now collapse to a single label. The large-KIE warning quotes the computed value and names the isotopologue explicitly.
A substitutions table after the species table spells out what each isotopologue means, resolving plain atom numbers to their element and default heavy isotope (e.g. 'H26 -> 2D'; multi-file runs label the per-file substitutions with the filename). New Hess_to_Freq.describe_substitutions helper.
Implements the last barrier-height tunneling item in the plan. The Skodje-Truhlar transmission coefficient (J. Phys. Chem. 1981, 85, 624) truncates the parabolic barrier at its actual height, so unlike the Bell infinite parabola it stays finite below the crossover temperature, and reduces to Bell as the barrier grows large above it. - thermo.skodje_truhlar_kappa; compute_isotope_effect(s) take an optional electronic_barrier (Hartree). Each isotopologue uses its own ZPE-corrected barrier, assembled from the electronic barrier and the RPFR ZPE sums already in hand. - CLI: --barrier to set it, otherwise assembled automatically from the files' SCF energies (assuming an exothermic reaction); a warning and skip if the barrier is non-positive. New κ S-T / KIE×κST columns shown only when a barrier is available. - IsotopeEffect gains st_correction / kie_st / barrier_light_j and JSON fields. Validated against an independent implementation to 1.4e-5 (residual traced to that code's constants). Honest caveat, now in the crossover warning: below T_c the S-T value is finite but still unreliable, not a fix for deep tunneling. Tests cover the S-T -> Bell limit above crossover and pin the HAT below-crossover values.
…neling models The intro paragraphs were stale: they described KIE-only, a hard-coded three-isotope table editable in Hess_to_Freq.py, and a single Bell tunneling correction. Reworded for KIE+EIE, the current isotope table (2D/3T, 13C/14C, 15N, 17O/18O with the atom:isotope syntax, in isotopes.py), and the Bell/Wigner/Skodje-Truhlar corrections with the crossover caveat. The Usage section was already current.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Kinisot keeps the Bigeleisen-Mayer math; file parsing and scaling factors now come from GoodVibes (new dependency, requires patonlab/GoodVibes#109 released as >= 4.4):
read_hess()mass-weights the Cartesian Hessian fromgoodvibes.io.parse_hessian— Gaussian archive block, or ORCA.hessfound next to the.out(or passed directly). First ORCA support in Kinisot.M06-2X/maug-cc-pVTZ→M062X/maug-cc-pV(T+d)Z); explicit-svalues are unaffected.Validation
examples/orca/), including the.hess-direct invocation path. 25 tests pass.Merge order
setup.pypinsgoodvibes>=4.4).Summary by CodeRabbit
.hessdiscovery (from.out) and direct.hessinput.--isosyntax for multi-isotopologue runs (includingatom:isotopesuffixes), plus--prdand--ref.kinisotconsole command (--version) and--json/--quietoutput options.IsotopeEffectobject with.to_dict().