Fix TypeError in check_files SPC level-of-theory validation#112
Conversation
check_files() takes level_of_theory as a list of per-file strings, but the SPC branch still called it as a function -- a leftover from before the refactor, when io.level_of_theory was imported at module level. Any run combining --check with --spc crashed with "TypeError: 'list' object is not callable". Import the io parser under an alias (read_level_of_theory) and use it to read each SPC file's level of theory. Also pass names_spc rather than the parent filenames to print_check_fails on the mismatch path, so groups are attributed to the right files when an SPC file is missing. Fixes #111
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughgoodvibes/validation.py now re-reads SPC level-of-theory values from the SPC files themselves during ChangesSPC level-of-theory validation fix
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
goodvibes/validation.py (1)
355-360: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid reading each SPC file twice.
level_of_theory(name)andparse_qcdata(name)both reopen the same SPC file; if SPC validation needs both the level and geometry, surface the level of theory fromparse_qcdata(or combine the two parsers) so each file is read once.🤖 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 `@goodvibes/validation.py` around lines 355 - 360, The SPC duplicate-check loop currently reads each file twice via level_of_theory(name) and parse_qcdata(name), so update the validation flow to reuse a single parse per file. Adjust parse_qcdata or its caller in the SPC handling path to return the level of theory alongside cartesians, then have the logic that builds geom_duplic_list_spc consume that combined result instead of reopening each SPC file.
🤖 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 `@goodvibes/validation.py`:
- Around line 330-336: Guard the SPC level-of-theory check in validation logic
so it only reports success when the SPC file list is complete and non-empty. In
the `l_o_t_spc` / `all_same` branch, add the same kind of length check used by
the geometry block before accessing `l_o_t_spc[0]`, and route empty or partial
`names_spc` cases to `print_check_fails` instead of logging the blanket success
message. This should be fixed in the SPC-reading section that uses
`read_level_of_theory`, `all_same`, and `print_check_fails`.
---
Nitpick comments:
In `@goodvibes/validation.py`:
- Around line 355-360: The SPC duplicate-check loop currently reads each file
twice via level_of_theory(name) and parse_qcdata(name), so update the validation
flow to reuse a single parse per file. Adjust parse_qcdata or its caller in the
SPC handling path to return the level of theory alongside cartesians, then have
the logic that builds geom_duplic_list_spc consume that combined result instead
of reopening each SPC file.
🪄 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: 3af0d489-fce8-487a-baf9-68650ba0a158
📒 Files selected for processing (2)
goodvibes/validation.pytests/test_validation.py
names_spc only collects SPC files that exist on disk, so it can be empty or shorter than the input file list (e.g. --spc link, or a missing _TZ partner file). all_same([]) returns True, so the empty case crashed with IndexError on l_o_t_spc[0], and a partial list logged a blanket success after checking only a subset. Mirror the length check the geometry block already uses: only run the success/mismatch branches when every input file has an SPC partner, otherwise log an explicit caution that files are missing.
bobbypaton
left a comment
There was a problem hiding this comment.
fixed per suggestion from coderabbit
Fixes #111
Problem
Combining
--checkwith--spc <suffix>crashed:check_files()takeslevel_of_theoryas a list of per-file strings, but the SPC-validation branch still contained a call from before the refactor, whenio.level_of_theory(a file parser) was imported at module level. The list parameter shadowed the function name. Not Windows-specific — reproduces on any platform.Fix
read_level_of_theory(aliased so the parameter no longer shadows it) and use it to read each SPC file's level of theory — the correct behavior, since the parameter only holds the freq-level strings.names_spc(the SPC filenames) toprint_check_failsinstead of the parent filenames, so groups are attributed correctly when an SPC file is missing.Testing
goodvibes ethane.out --spc TZ --check; after the fix the run completes and reportsUsing B3LYP/6-311G(d,p) in all the single-point corrections(the TZ single-point level, distinct from the B3LYP/6-31G(d) freq level).test_check_files_spc_reads_spc_level_of_theory.Summary by CodeRabbit