Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds support for optional 4-digit year suffixes to distinguish method variants in the Arkane database (e.g., b97d3 vs b97d32023). The implementation allows users to omit the year (ARC matches the latest available), specify an explicit year, or receive warnings when a requested year is unavailable.
Key changes:
- Added
yearparameter to theLevelclass with validation requiring 4-digit integers - Implemented flexible method/basis matching logic with year-aware searching in Arkane database
- Enhanced error reporting to list available years when a requested year is not found
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/source/advanced.rst | Documents the new year parameter with usage examples |
| arc/level.py | Adds year attribute with 4-digit validation and updates string representations |
| arc/level_test.py | Adds test for year validation (rejects 2-digit, accepts 4-digit) |
| arc/statmech/arkane.py | Core implementation: adds normalization functions, year-aware matching logic, and improved error messages; also adds spinMultiplicity to non-SMILES species template and thread limit environment variables |
| arc/statmech/arkane_test.py | Adds test for year-not-found warning behavior and year in _level_to_str |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
7955fd4 to
67e2cfe
Compare
1926671 to
773ab92
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated 11 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
docs/source/advanced.rst
Outdated
| and are not valid QC methods. Do not include year suffixes in ``level_of_theory``; instead, set | ||
| ``arkane_level_of_theory`` with a ``year`` value if you need a specific correction year. |
There was a problem hiding this comment.
The documentation incorrectly advises users to set year in arkane_level_of_theory, but based on the PR description and implementation, the year parameter should be set directly on sp_level (or other level arguments like freq_level, opt_level). The arkane_level_of_theory is typically derived from sp_level unless explicitly overridden. Users should be advised to set the year parameter on sp_level or other level_of_theory parameters, not on arkane_level_of_theory specifically.
| and are not valid QC methods. Do not include year suffixes in ``level_of_theory``; instead, set | |
| ``arkane_level_of_theory`` with a ``year`` value if you need a specific correction year. | |
| and are not valid QC methods. Do not include year suffixes in ``level_of_theory``; instead, specify a | |
| ``year`` key on ``sp_level`` (and/or other job-specific level dictionaries such as ``opt_level``, | |
| ``freq_level``, or ``scan_level``) if you need a specific correction year. |
docs/source/examples.rst
Outdated
| set ``arkane_level_of_theory`` with a ``year`` value instead. | ||
| If ``year`` is omitted, ARC will prefer the no-year Arkane entry for that method/basis; if none exists, | ||
| ARC will fall back to the latest available year in the Arkane database. |
There was a problem hiding this comment.
The documentation incorrectly advises users to set year in arkane_level_of_theory. Based on the PR implementation and examples shown earlier in the documentation (lines 98-112), the year parameter should be set directly on sp_level or other level parameters (opt_level, freq_level, etc.), not specifically on arkane_level_of_theory. The arkane_level_of_theory is typically auto-derived from sp_level unless explicitly overridden.
| set ``arkane_level_of_theory`` with a ``year`` value instead. | |
| If ``year`` is omitted, ARC will prefer the no-year Arkane entry for that method/basis; if none exists, | |
| ARC will fall back to the latest available year in the Arkane database. | |
| set a ``year`` field on the relevant level parameter (e.g., ``sp_level``, ``opt_level``, ``freq_level``, | |
| etc.) rather than on ``level_of_theory`` or ``arkane_level_of_theory``. If ``year`` is omitted, ARC will | |
| prefer the no-year Arkane entry for that method/basis; if none exists, ARC will fall back to the latest | |
| available year in the Arkane database. |
arc/statmech/arkane.py
Outdated
| f"available years: {_format_years(years)}. " | ||
| f"Specify a year to select a matching entry." | ||
| ) | ||
| return _level_to_str(sp_level) |
There was a problem hiding this comment.
Inconsistent behavior when Arkane AEC entry is not found: For composite methods (line 978), the function returns _level_to_str(sp_level) even when no matching entry exists in the database. However, for non-composite methods with freq_scale_factor (line 1005), it returns None. This inconsistency could lead to Arkane attempting to use a level of theory that lacks proper corrections. Both cases should handle missing entries consistently, likely by returning None after logging the warning.
| return _level_to_str(sp_level) | |
| # No matching AEC level in Arkane DB for this composite method | |
| return None |
data/quantum_corrections.py
Outdated
| # Iodine SOC calculated as a weighted average of the electronic spin splittings of the lowest energy state. | ||
| # The splittings are obtained from Huber, K.P.; Herzberg, G., Molecular Spectra and Molecular Structure. IV. | ||
| # Constants of Diatomic Molecules, Van Nostrand Reinhold Co., 1979 | ||
| # Spin orbit correction for F, Si, Cl, Br and B taken form https://cccbdb.nist.gov/elecspin.asp |
There was a problem hiding this comment.
Spelling error: 'form' should be 'from'
| # Spin orbit correction for F, Si, Cl, Br and B taken form https://cccbdb.nist.gov/elecspin.asp | |
| # Spin orbit correction for F, Si, Cl, Br and B taken from https://cccbdb.nist.gov/elecspin.asp |
arc/statmech/arkane.py
Outdated
| def _available_years_for_level(level: "Level", | ||
| file_path: str, | ||
| section_start: str, | ||
| section_end: str) -> list[Optional[int]]: |
There was a problem hiding this comment.
Type hint inconsistency: This function uses lowercase list[Optional[int]] for the return type, but the file imports and uses List from typing module elsewhere (see line 9 and line 683). For consistency with the rest of the codebase, use List[Optional[int]] instead of list[Optional[int]].
arc/statmech/arkane.py
Outdated
| return sorted(years, key=lambda y: (-1 if y is None else y)) | ||
|
|
||
|
|
||
| def _format_years(years: list[Optional[int]]) -> str: |
There was a problem hiding this comment.
Type hint inconsistency: This function parameter uses lowercase list[Optional[int]], but the file imports and uses List from typing module elsewhere (see line 9 and line 683). For consistency with the rest of the codebase, use List[Optional[int]] instead of list[Optional[int]].
| def _format_years(years: list[Optional[int]]) -> str: | |
| def _format_years(years: List[Optional[int]]) -> str: |
arc/statmech/arkane.py
Outdated
|
|
||
| target_method_norm = _normalize_method(level.method) | ||
| target_base, method_year = _split_method_year(target_method_norm) | ||
| target_year = getattr(level, 'year', None) if getattr(level, 'year', None) is not None else method_year |
There was a problem hiding this comment.
The code allows year suffixes in method names (e.g., 'b97d32023') and extracts them for matching, but the documentation explicitly advises against including year suffixes in method names. Consider adding validation to warn or raise an error if a user provides both a year suffix in the method name AND an explicit year parameter with conflicting values, to prevent user confusion and potential bugs.
| target_year = getattr(level, 'year', None) if getattr(level, 'year', None) is not None else method_year | |
| explicit_year = getattr(level, 'year', None) | |
| if explicit_year is not None and method_year is not None and explicit_year != method_year: | |
| raise InputError( | |
| f"Conflicting year specifications for level '{level}': " | |
| f"explicit year={explicit_year}, method suffix year={method_year}. " | |
| "Please remove the year suffix from the method name or update the 'year' attribute to match." | |
| ) | |
| target_year = explicit_year if explicit_year is not None else method_year |
arc/statmech/arkane.py
Outdated
| If multiple entries only differ by year, the one with the *latest* year | ||
| is chosen (year=0 if no year in that entry). |
There was a problem hiding this comment.
The docstring description at lines 936-937 is misleading. It states that "the one with the latest year is chosen" but this only applies when the user does NOT specify a year AND no no-year entry exists. When a user specifies a year explicitly via the year parameter, only that exact year will be matched. The docstring should clarify that the latest-year fallback only occurs when no year is specified and no no-year entry exists.
| If multiple entries only differ by year, the one with the *latest* year | |
| is chosen (year=0 if no year in that entry). | |
| When a year is explicitly specified in the Level, only entries with that exact | |
| year are matched. If no year is specified and an entry without a year exists, | |
| that entry is used. Only when no year is specified and no no-year entry exists, | |
| if multiple entries differ only by year, the one with the *latest* year is | |
| chosen (treating entries with no year as year=0). |
arc/statmech/arkane.py
Outdated
| qm_corr_files = _get_qm_corrections_files() | ||
| if not qm_corr_files: | ||
| return None |
There was a problem hiding this comment.
When no quantum corrections files are found (qm_corr_files is empty), the function silently returns None without any warning or error message. Consider adding a logger.warning() to inform users that no quantum corrections database files were found, which would help with debugging configuration issues.
aeabe6c to
8dee498
Compare
0a127ff to
331c440
Compare
|
@calvinp0, can you please see which of the copilot comments are relevant? |
331c440 to
ac61569
Compare
611490b to
43ca9df
Compare
Added the input parameter year so that the year of the level of theory can be defined (if required). Added a test where it ensures that the year provided is 4 digits and not 2.
When a user provides in the input file (for example) ```yaml level_of_theory: wb97xd/def2tzvp ``` They can also provide ```yaml level_of_theory_year: 2023 ``` If they want to be specific about the year LoT (Or provide None - therefore ensuring no match to any year) Also we check that there is a conflict between LoTY and sp_level year, although that would not be permissable anyway. Added tests to ensure sp_level respects the year and opt_level does not need. Added test o coerce the year from string added test for explicit year in sp_level added tests for year in arkane_level_theory year
Intention of the addtional logic and function is to match the users year in LoTY along with the LoT to the correct quantum correction from RMG-database. However, if the user omits the LoTY - then it will be assumed that the latest year available in the quantum corrections is the one to be used. We also provide feedback now if the year req. was not present in the quantum corrections
Documentation now provides information about the year matching
43ca9df to
9b3e0fe
Compare
Introduces a function to generate unique project names for parallel test execution using xdist. This prevents collisions when cleaning up project directories, ensuring reliable test execution.
allows for `pip install .` or `pip install -e .`
alongd
left a comment
There was a problem hiding this comment.
Thanks! I added a couple of comments, both on the same topic
| e.g., 'ZINDO/2', 'DLPNO-MP2-F12/D' | ||
| For these cases, use the dictionary-type job-specific level of theory arguments | ||
| instead (e.g., ``opt_level``). | ||
| level_of_theory_year (int or str, optional): An optional 4-digit year suffix to apply to the single-point |
There was a problem hiding this comment.
I think this should not be an arg of ARC. Instead, ARC has individual Level objects for sp_level, opt_liver, etc., and each one has a year attr as you implemented in the prev commit. I recommend removing this from main
| for Arkane database matching only and are not valid QC methods. If you need a specific correction year, | ||
| specify either: | ||
|
|
||
| - ``level_of_theory_year: 2023`` (applies to the SP side of ``level_of_theory``), or |
There was a problem hiding this comment.
can we instead only give the year in the Level argument, and then use it just for Arkane and not for the ESS calcs?
RMG-database for AEC has the suffix of years for methods and this can cause issues for ARC when it attempts to find a method/basis if the user does not provide the year. This PR is meant to allow for the following:
This pull request introduces support for specifying an optional 4-digit year suffix for levels of theory, allowing users to differentiate between method variants (e.g.,
b97d3vsb97d32023). The changes include enhancements to theLevelclass, updates to the ARC main workflow to handle the newyearparameter, improved input validation and coercion, and expanded test coverage to ensure correct behavior and error handling.Level of Theory Year Support
yearparameter to theLevelclass, including validation to ensure it is a 4-digit integer, and incorporated it into string representations (__str__andsimplemethods). (arc/level.py) [1] [2] [3] [4] [5]buildmethod ofLevelto include theyearkey in its dictionary representation. (arc/level.py)ARC Main Workflow Enhancements
level_of_theory_yearparameter to theARCclass, with input validation and coercion logic to handle integer or string inputs, and updated documentation/comments accordingly. (arc/main.py) [1] [2] [3] [4] [5]process_level_of_theoryto correctly apply and validate the year suffix, including error handling for conflicting year specifications and ensuringlevel_of_theory_yearis only used withlevel_of_theory. (arc/main.py) [1] [2]Testing and Documentation
arc/level_test.py,arc/main_test.py,arc/statmech/arkane_test.py) [1] [2] [3] [4]yearparameter and its usage in input files. (docs/source/advanced.rst)Arkane Integration and Helper Functions
arc/statmech/arkane_test.py) [1] [2] [3]These changes collectively improve ARC's flexibility and robustness in handling method variants distinguished by year, and ensure consistent integration with Arkane databases and user inputs.