ReactionMechanismGenerator / ReactionMechanismGenerator/T3
PFR flux: surface (heterogeneous) reaction support is broken
Nobody has claimed this yet.
- Dominant language
- Python
- Stars
- 14
- Forks
- 7
- Avg merge
- 22h 36m
- Merged PRs (30d)
- 22
Description
Correction (see history): this issue was first filed on the assumption that the surface commit would be dropped from #162 before merge. It was not — #162 merged with all four commits, so
34de194and the defects below are inmainnow. This is a bug report, not a feature request.
#162 added a PFR reactor to the flux diagram generator. Its surface-reaction support does not work: passing a non-empty surface_names raises, and even past the raise the numbers it would report are meaningless.
The defects
Both in t3/utils/flux.py, introduced by 34de194:
-
ct.Interfaceis not called that way.run_pfrdoessurface(surface_names.index(surface.name)).thermo, which raises as soon assurface_namesis non-empty. The gas-phase path is unaffected —surface_namesdefaults toNoneand nothing reaches this block. -
The instances read are not the instances simulated. This is the deeper one.
set_pfrconstructs its ownct.Interfaceper cell:for surface in surfaces: surface_instance = ct.Interface(model_path, name=surface.name, phases=[gas_cell]) ct.ReactorSurface(surface_instance, reactor)and attaches those to the reactors, while
run_pfrreads rates off the outersurfaceslist it built separately. Those objects are never attached to any reactor and never advanced, so even with (1) fixed the reported ROPs would come from an unintegrated phase still at inlet conditions.
What a fix needs
set_pfrto return thect.ReactorSurface/ct.Interfaceobjects it creates, sorun_pfrreads rates from the phases that were actually integrated — presumably from the last cell, to match how the gas-phase outlet state is taken.- A decision on which cell's surface state the diagram should represent (outlet only, or summed along the reactor).
- A surface-bearing test fixture. Nothing in
tests/data/models/carries a surface phase, which is why this path is untested in either direction and why the defects survived tomain. A small gas+surface mechanism would also lettest_pfr_physics_and_profilesbe extended.
Interim
Until it is fixed, surface_names should be treated as unsupported. Worth considering an explicit NotImplementedError when it is passed, so it fails with a clear message rather than an ct.Interface object-not-callable error.
Context
- Cantera pin is
>=3.2.0(environment.yml). The surface code was written against a pre-3.x API — the same drift that leftct.PressureController(master=...)inset_pfr, fixed in #162 before merge.
Contributor guide
No contributing guide indexed for this repository
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start in t3/utils/flux.py, comparing set_pfr with run_pfr, then inspect tests/data/models/ and test_pfr_physics_and_profiles. Add or use a gas-plus-surface fixture, decide whether the diagram represents outlet or summed surface state, and verify the supported path reports rates from integrated reactor surfaces without breaking gas-only tests.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python
- Domain
- backend, testing-qa
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 48/100