Conversation
JacksonBurns
approved these changes
Sep 14, 2026
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved validation and adiabatic-warning issues must be addressed before approval.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds warnings for heuristic auto-database selection, documents its limitations, and strengthens surface database validation.
Changes:
- Warns about unsupported elements, adiabatic reactors, and omitted PAH libraries.
- Adds warning tests and documentation.
- Extends surface database validation.
File summaries
| File | Summary |
|---|---|
test/rmgpy/data/autoDatabaseTest.py |
Adds auto-database warning tests; positive adiabatic-warning coverage is still needed. |
test/database/databaseTest.py |
Extends surface metadata validation; optional facets and malformed values need safer handling. |
rmgpy/data/auto_database.py |
Adds coverage and reactor warnings; temperature reporting and positive test coverage need updates. |
documentation/source/users/rmg/input.rst |
Documents auto-selection limitations and warnings. |
Review details
Suppressed comments (1)
test/rmgpy/data/autoDatabaseTest.py:735
- This only covers the negative case with an isothermal
SimpleReactor; the new positive branch atrmgpy/data/auto_database.py:295-303is never exercised. Add a test that constructs aConstantVIdealGasReactor(or patchesRMSConstV) and asserts that the adiabatic warning is emitted, so regressions in the class check or optional import path are caught.
def test_isothermal_reactor_does_not_warn_about_adiabatic_temperature(self):
profile = ChemistryProfile(elements_present={'C', 'H'}, has_carbon=True,
max_temperature=1000.0)
messages = self._warnings(profile, [_simple_reactor(1000.0)])
self.assertNotIn('adiabatic reactor', messages)
- Files reviewed: 4/4 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+474
to
478
| if not entry.facet: | ||
| logging.error(f"Expected a facet attribute for {entry} in {family} family but found {entry.facet!r}") | ||
| failed = True | ||
| else: | ||
| with check: |
Comment on lines
+297
to
+302
| f' An adiabatic reactor is present, so the detected maximum temperature ' | ||
| f'({profile.max_temperature:.0f} K) is the initial temperature and the true ' | ||
| f'peak temperature will be higher. Temperature-gated library sets were ' | ||
| f'evaluated at the initial temperature; if the peak is expected to exceed ' | ||
| f'{CH_PYROLYSIS_T_THRESHOLD:.0f} K, consider requesting the high-temperature ' | ||
| f'libraries explicitly.' |
|
|
||
| # A metal+facet must resolve to a real metal library entry so that | ||
| # binding energies can be looked up without ambiguity. | ||
| if entry.metal and entry.facet: |
| f'selected; add any relevant libraries to the database() block manually.' | ||
| ) | ||
|
|
||
| if RMSConstV is not None and any(isinstance(r, RMSConstV) for r in reaction_systems): |
Member
Author
|
Copilot review caught something. The facet stuff was meant to be on a different branch, something else that was being tested. Got swept up by git. Don't merge this yet until I get a chance to sort it out. |
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
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.
Motivation or Problem
A reviewer of the RMG4 paper suggested the auto-selection feature should include warnings when one strays out of bounds.
Description of Changes
A few scenarios are detected and warned about in the logs.
Also added a paragraph of generic warning to the documentation.
Also rolled in a small extension to an error message for database tests.
Testing
Some unit tests are created for the warnings.
AI Use
Claude Code (Opus 5) wrote most of this.
I edited lightly.