Conversation
alongd
force-pushed
the
migration
branch
8 times, most recently
from
2b333fd to
506a4b7
Compare
There was a problem hiding this comment.
Pull request overview
This PR performs a broad “T3 Migration” overhaul focused on reducing direct RMG-Py coupling, standardizing simulation adapters around a shared Cantera base, and modernizing packaging/CI + test fixtures to match the new architecture.
Changes:
- Introduces Cantera-based simulate adapters (TP/HP/UV + PFR) with a shared
CanteraBase, plus a lightweight Cantera YAML parser and an RMG SA CSV parser that avoidrmgpyat runtime. - Refactors the RMGConstantTP adapter to run SA via an incore subprocess script and YAML output, and updates writer/schema/tests accordingly.
- Updates repo packaging/CI/devtools and refreshes test data/fixtures for the new simulate + SA workflows.
Reviewed changes
Copilot reviewed 76 out of 94 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_utils/test_writer.py | Updates expected RMG input writing behavior (pydantic model_dump, simulator args, adapter name changes). |
| tests/test_utils/test_libraries.py | Adds new unit tests for library append + atomic dictionary update behavior using shim APIs. |
| tests/test_utils/test_generator.py | Migrates radical generation tests from rmgpy.Species to T3Species. |
| tests/test_utils/test_fix_cantera.py | Adds tests for Cantera file fix utilities + traceback parsing. |
| tests/test_utils/test_cantera_parser.py | Adds test coverage for new Cantera YAML parser (species/reactions/thermo comment parsing). |
| tests/test_simulate/test_cantera_constant_uv.py | Adds Cantera constant-U/V adapter simulation tests. |
| tests/test_simulate/test_cantera_constant_hp.py | Adds Cantera constant-H/P adapter simulation tests. |
| tests/test_simulate/test_cantera_base.py | Adds comprehensive tests for shared CanteraBase behavior and regression fixes. |
| tests/test_simulate/data/rms_simulator_test/input.yml | Adds RMS simulator test input fixture. |
| tests/test_simulate/data/rmg_simulator_test/iteration_0/SA/solver/simulation_1_12.csv | Adds RMG SA fixture output (simulation profile). |
| tests/test_simulate/data/rmg_simulator_test/iteration_0/SA/solver/sensitivity_1_SPC_4.csv | Adds RMG SA fixture output (OH observable). |
| tests/test_simulate/data/rmg_simulator_test/iteration_0/SA/solver/sensitivity_1_SPC_3.csv | Adds RMG SA fixture output (H observable). |
| tests/test_simulate/data/rmg_simulator_test/iteration_0/RMG/solver/simulation_1_12.csv | Adds RMG fixture output (simulation profile). |
| tests/test_simulate/data/rmg_simulator_test/iteration_0/RMG/input.py | Updates stored RMG input fixture (simulator/options changes). |
| tests/test_simulate/data/rmg_simulator_test/iteration_0/RMG/chemkin/species_dictionary.txt | Adds fixture species dictionary for RMG/Cantera tests. |
| tests/test_simulate/data/rmg_simulator_test/input.yml | Adds RMG simulator test input fixture. |
| tests/test_simulate/data/cantera_simulator_test/iteration_0/RMG/input.py | Adds fixture RMG input used by Cantera simulate tests. |
| tests/test_simulate/data/cantera_simulator_test/iteration_0/RMG/chemkin/species_dictionary.txt | Adds fixture species dictionary used by Cantera simulate tests. |
| tests/test_simulate/data/cantera_simulator_test/input.yml | Adds Cantera simulator test input fixture. |
| tests/test_simulate_adapters/test_rmg_constantTP.py | Removes legacy adapter tests (superseded by new simulate test suite). |
| tests/test_simulate_adapters/data/rmg_simulator_test_ranges_3c/t3.log | Removes legacy log fixture. |
| tests/test_schema.py | Updates schema tests for adapter defaults and termination_time typing. |
| tests/test_runners/test_rmg_runner.py | Updates expected submit/run commands from python-jl to python. |
| tests/test_libraries.py | Removes old rmgpy-dependent library tests (replaced by shim-based tests). |
| tests/test_functional.py | Updates functional tests/log assertions for the migrated workflow and makes backup test more isolated. |
| tests/test_common.py | Updates common tests to use T3Species and adds YAML key-ordering + /dH[ SA header parsing tests. |
| tests/data/pdep_network/iteration_1/PDep_SA/network4_2/CSE/chem.inp | Adds/updates fixture file for pdep network tests. |
| tests/data/models/test_species_dictionary.txt | Adds fixture RMG species dictionary for Cantera parser tests. |
| tests/data/models/test_cantera_parser.yaml | Adds fixture Cantera YAML for parser tests. |
| tests/data/functional_2_thermo/input.yml | Renames functional test project fixture. |
| tests/common.py | Updates test helpers (ARC Molecule import, radical checks, forces CanteraConstantTP in run_minimal). |
| tests/check_t3_path.py | Adds debug helper script to print import path. |
| t3/utils/writer.py | Refactors species object creation to T3Species, fixes tolerance fallback logic, and adds get_species_obj_from_a_species_dict. |
| t3/utils/rmg_sa_parser.py | Adds a lightweight RMG SA CSV parser and converter to standardized sa_dict without rmgpy. |
| t3/utils/generator.py | Migrates radical generation implementation to ARC molecule objects + T3Species. |
| t3/utils/flux.py | Updates defaults/API usage for Cantera and simplifies ROP accumulation logic + graph output writing. |
| t3/utils/dependencies.py | Simplifies dependency checks (ARC-only in t3_env). |
| t3/utils/cantera_parser.py | Adds a lightweight Cantera YAML parser that produces T3Species/T3Reaction. |
| t3/simulate/rmg_constantTP.py | Removes legacy in-process RMG adapter implementation. |
| t3/simulate/rmg_constant_tp.py | Adds new RMGConstantTP adapter that runs SA via subprocess incore script and YAML output. |
| t3/simulate/factory.py | Minor refactor of factory construction and docstrings. |
| t3/simulate/cantera_pfr.py | Adds a Cantera PFR adapter (lagrangian + chain-of-reactors modes). |
| t3/simulate/cantera_constant_uv.py | Adds Cantera constant UV adapter. |
| t3/simulate/cantera_constant_tp.py | Adds Cantera constant TP adapter. |
| t3/simulate/cantera_constant_hp.py | Adds Cantera constant HP adapter. |
| t3/simulate/init.py | Updates simulate module exports to new adapter modules/base. |
| t3/settings/t3_submit.py | Updates template scripts to run via python instead of python-jl. |
| t3/runners/rmg_runner.py | Updates incore RMG command to python and adds Arkane runner + RMG SA incore runner helper. |
| t3/runners/rmg_incore_script.py | Updates docs/exception list to match current RMG errors and python invocation. |
| t3/runners/rmg_incore_sa.py | Adds incore RMG SA runner script producing YAML output. |
| t3/logger.py | Migrates logger summaries to use T3Species/T3Reaction objects and adds clean_t3_status. |
| t3/common.py | Bumps version, adds SA (de)serialization helpers, adds YAML saving with key ordering, updates SA header parsing. |
| requirements.txt | Updates python dependency set (adds Cantera, bumps numpy/cython/rdkit, etc.). |
| pyproject.toml | Adds packaging metadata for setuptools build. |
| Makefile | Updates install targets (renames PyRMS to PyRDL install). |
| ipython/sa_coefficients_viewer.ipynb | Adds notebook for visualizing SA coefficient YAML output. |
| examples/minimal/input.yml | Switches minimal example SA adapter default and updates QM job_types fields. |
| environment.yml | Major environment modernization (Python 3.12+, Cantera 3.2+, numpy 2.1+, revised channels/deps). |
| devtools/install_pyrms.sh | Removes old PyRMS installer. |
| devtools/install_pyrdl.sh | Adds wrapper installer delegating to ARC’s PyRDL installer. |
| devtools/install_arc.sh | Removes old combined ARC/RMG installation script. |
| devtools/install_all.sh | Updates “install all” workflow and adds PyRDL installation step. |
| .github/workflows/cont_int.yml | Removes legacy CI workflow. |
| .github/workflows/ci.yml | Adds new CI workflow using micromamba + ARC + RMG-database checkout. |
Comments suppressed due to low confidence (1)
t3/utils/flux.py:195
get_profiles_from_simulation()now defaultsV=100, butgenerate_flux()always forwards itsVargument (defaulting toNone). For JSR runs this still passesNoneintorun_jsr()/set_jsr()and will raise aTypeErrorwhen computingvolume=V * 1e-6. Consider treatingV=Noneas “use default volume” (e.g., only passVthrough when it’s notNone, or defaultgenerate_flux(..., V=100)/ handleNoneinsideset_jsr).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 89 out of 111 changed files in this pull request and generated 14 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| clean_eq = re.sub(r'\(\+[^)]*(?:\([^)]*\))?\)', '', equation) | ||
| reactants_str, products_str = clean_eq.split(arrow) |
There was a problem hiding this comment.
clean_eq.split(arrow) will raise ValueError if arrow is not present in the equation string (e.g., equations using = but not <=>/=>). Also, the rxn.label = ... lines appear mis-indented (extra leading space), which can cause an IndentationError depending on the actual whitespace. Consider (1) including '=' in the arrow detection before splitting (or using a regex split on all supported arrows), and (2) normalizing indentation to consistent 4-space blocks.
| if '<=>' in equation: | ||
| rxn.label = equation | ||
| elif '=>' in equation: | ||
| rxn.label = equation.replace('=>', '<=>') | ||
| elif '=' in equation: | ||
| rxn.label = equation.replace('=', '<=>') | ||
| else: | ||
| rxn.label = equation # Fallback |
There was a problem hiding this comment.
clean_eq.split(arrow) will raise ValueError if arrow is not present in the equation string (e.g., equations using = but not <=>/=>). Also, the rxn.label = ... lines appear mis-indented (extra leading space), which can cause an IndentationError depending on the actual whitespace. Consider (1) including '=' in the arrow detection before splitting (or using a regex split on all supported arrows), and (2) normalizing indentation to consistent 4-space blocks.
| seen_equations = set() | ||
| for i, rxn_datum in enumerate(reactions_data): | ||
| equation = rxn_datum.get('equation') | ||
| if not equation: | ||
| continue | ||
| # Skip duplicate reaction entries (same equation appears multiple times | ||
| # for reactions with multiple rate expressions that are summed). | ||
| if equation in seen_equations: | ||
| continue | ||
| seen_equations.add(equation) |
There was a problem hiding this comment.
Skipping reactions solely because the equation string repeats can silently drop valid Cantera reactions (e.g., true duplicates that should be summed, or multiple rate expressions that must be preserved to reproduce kinetics). A safer approach is to keep all reaction entries and mark duplicates explicitly (or aggregate rate expressions in a controlled way) rather than discarding them.
| seen_equations = set() | |
| for i, rxn_datum in enumerate(reactions_data): | |
| equation = rxn_datum.get('equation') | |
| if not equation: | |
| continue | |
| # Skip duplicate reaction entries (same equation appears multiple times | |
| # for reactions with multiple rate expressions that are summed). | |
| if equation in seen_equations: | |
| continue | |
| seen_equations.add(equation) | |
| for i, rxn_datum in enumerate(reactions_data): | |
| equation = rxn_datum.get('equation') | |
| if not equation: | |
| continue | |
| # Preserve all reaction entries, even when the equation string repeats. | |
| # In Cantera YAML, repeated equations can represent valid duplicate | |
| # reactions or multiple rate expressions that must be retained. |
| if rxn.duplicate and rxn.equation not in dups: | ||
| dups.append(rxn.equation) | ||
| profile = {'P': gas.P, 'T': gas.T, 'X': {s.name: x for s, x in zip(gas.species(), gas.X)}, 'ROPs': rops} | ||
| rops_snapshot[spc.name][rxn.equation] = cantera_reaction_rops[i] * stoichiometry[spc.name][i] |
There was a problem hiding this comment.
rops_snapshot[spc.name][rxn.equation] = ... overwrites values when multiple Cantera reactions share the same rxn.equation (duplicate reactions), losing contributions instead of summing them. This is a behavior regression vs. the prior logic that accumulated duplicate equations. Use accumulation (e.g., +=) keyed by equation, or store per-reaction-index keys to avoid collisions.
| rops_snapshot[spc.name][rxn.equation] = cantera_reaction_rops[i] * stoichiometry[spc.name][i] | |
| rop_contribution = cantera_reaction_rops[i] * stoichiometry[spc.name][i] | |
| rops_snapshot[spc.name][rxn.equation] = ( | |
| rops_snapshot[spc.name].get(rxn.equation, 0.0) + rop_contribution | |
| ) |
| for val_0, val_1 in zip(spc_0_vals, spc_1_vals[::-1]): | ||
| new_species_list = species_list | ||
| new_species_list.append({'label': self.rmg['species'][spc_indices_w_ranges[0]]['label'], | ||
| 'concentration': val_0}) | ||
| new_species_list.append({'label': self.rmg['species'][spc_indices_w_ranges[1]]['label'], | ||
| 'concentration': val_1}) | ||
| species_lists.append(new_species_list) |
There was a problem hiding this comment.
new_species_list = species_list reuses the same list object and mutates it across iterations, causing concentrations/species entries to accumulate and pollute subsequent cases. Additionally, in the modify_concentration_ranges_together branch, species_lists.append(new_species_list) is outside the for point_number loop, so only the final mutated list is appended once. Use a fresh copy per combination (e.g., list(species_list)), and append inside the loop for the “together” case.
| @@ -0,0 +1,3 @@ | |||
|
|
|||
| import t3 | |||
| print(f"DEBUG: T3 imported from {t3.__file__}") | |||
There was a problem hiding this comment.
This debug helper script lives under tests/ and will be packaged/checked-in as part of the test suite even though it isn’t a test. Consider moving it to devtools/ (or guarding it under if __name__ == "__main__":) to avoid accidental execution and to keep the tests/ directory focused on automated tests.
| print(f"DEBUG: T3 imported from {t3.__file__}") | |
| if __name__ == "__main__": | |
| print(f"DEBUG: T3 imported from {t3.__file__}") |
| arc_available = True | ||
| try: | ||
| from arc import ARC | ||
| except ImportError: | ||
| arc_available = False | ||
|
|
||
| if not rmg_available or not arc_available: | ||
| if not arc_available: | ||
| msg = 'Error: Cannot execute T3. Missing the following critical component(s):\n' | ||
| if not rmg_available: | ||
| msg += ' - RMG\n' | ||
| msg += ' (See https://reactionmechanismgenerator.github.io/RMG-Py/users/rmg/installation/index.html)\n' | ||
| if not arc_available: | ||
| msg += ' - ARC\n' | ||
| msg += ' (See https://reactionmechanismgenerator.github.io/ARC/installation.html)\n' | ||
| msg += ' - ARC\n' | ||
| msg += ' (See https://reactionmechanismgenerator.github.io/ARC/installation.html)\n' | ||
| msg += '\nInstall the missing packages, make sure they were added to the PYTHONPATH, and rerun T3.' |
There was a problem hiding this comment.
check_dependencies() no longer checks for RMG availability, but T3 still requires RMG (even if executed in a separate env/subprocess). This can cause later failures with less actionable errors. Consider adding an explicit check for the expected RMG execution mode (e.g., presence of rmgpy when running incore, or presence of rmg_env/rmg.py path when running via subprocess) and emitting a targeted message.
| def rmg_sa_csvs_to_sa_dict(solver_dir: str, | ||
| chem_annotated_path: Optional[str] = None, | ||
| ) -> dict: |
There was a problem hiding this comment.
This new conversion entry point is core to producing standardized sa_dict output from RMG SA CSVs, but there aren’t corresponding unit tests in this PR. Since the repo includes SA CSV fixtures under tests/test_simulate/data/.../solver/, add tests that validate (1) time parsing, (2) kinetics/thermo key extraction, and (3) reaction-index remapping via chem_annotated_path.
| T: float, | ||
| P: float, | ||
| V: Optional[float] = None, | ||
| V: Optional[float] = 100, |
There was a problem hiding this comment.
Changing the default reactor volume from None to 100 (with unclear units) is a behavior change that can significantly alter simulation results (residence time, dilution, etc.) and may mask user misconfiguration. Consider keeping the default as None (and requiring explicit volume for volume-dependent reactors), or document/enforce units and use a physically reasonable default.
| V: Optional[float] = 100, | |
| V: Optional[float] = None, |
tests/test_utils/test_fix_cantera.py
Outdated
| import os | ||
| import shutil | ||
|
|
||
| from t3.common import t3_path, TEST_DATA_BASE_PATH |
There was a problem hiding this comment.
t3_path is imported but not used anywhere in this test module. Removing it avoids confusion and keeps the test module minimal.
| from t3.common import t3_path, TEST_DATA_BASE_PATH | |
| from t3.common import TEST_DATA_BASE_PATH |
removed commented out code, added run_arkane_job()
- Configure build system using setuptools and wheel. - Define project metadata (name, description, python requires). - Enable dynamic versioning reading from `t3.common.VERSION`. - Explicitly configure package discovery to include `t3*`. - Allow `pip install -e .` to function correctly in development and CI.
And updated simulate __init__.py
Added dependabot, removed labeler updated codeql, gh-pages
An overhaul of the T3 repo