-
-
Notifications
You must be signed in to change notification settings - Fork 361
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
FlowReactor with heterogeneous chemistry #1490
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1490 +/- ##
==========================================
+ Coverage 69.77% 70.29% +0.51%
==========================================
Files 377 376 -1
Lines 58011 58242 +231
Branches 20695 20797 +102
==========================================
+ Hits 40479 40943 +464
+ Misses 14577 14315 -262
- Partials 2955 2984 +29
... and 2 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
8fb5fe4
to
c287969
Compare
6321064
to
76a05a4
Compare
5af9499
to
25db091
Compare
d7b2a31
to
b4aadc0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@speth ... thank you for tackling this PR, which concludes a major effort started some time ago. I mainly have a couple of minor comments, plus the concern that removing distance
may be confusing for some.
samples/python/reactors/surf_pfr.py
Outdated
0, *r.thermo['CH4', 'H2', 'CO'].X)) | ||
|
||
while sim.time < length: | ||
dist = sim.time * 1e3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that distance is proportional to time, but it's still a little confusing to write it like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made some changes so that ReactorNet
now has both time
and distance
properties, where the one that is not applicable (depending on the reactor types involved) raises an exception. I updated some of the documentation as well to reflect the fact that the integrator's independent variable could be either time or distance, but stopped short of creating alternative names for set_initial_time()
and the max_time_step
property.
} | ||
double distance() const { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you reconsider the removal of distance
? In some contexts (examples!), using time as a placeholder for distance is somewhat confusing, and also not universally applicable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I brought this back, but in a deprecated form -- I think the value of the independent variable should be accessed via the ReactorNet
object, the same as for other Reactor
types.
property distance: | ||
""" The distance of the fluid element from the inlet of the reactor.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See other comment.
… Reactor & Add python example
Sundials versions Co-authored-by: Nick Curtis <arghdos@gmail.com>
Some implementation issues are fixed here, but the sensitivities currently do not match finite difference comparisons.
The kinetic data is only valid for a single temperature, so the example should be at constant temperature to avoid being misleading. Update plots to show deposition rates
The SurfPhase object needs to be reset to match the reactor surface's input state before calculating the consistent initial conditions, in case the SurfPhase state was altered between creating the ReactorSurface and initializing the ReactorNet.
Rename DAE-specific 'getState' to 'getStateDae' to avoid ambiguity when overriding.
Also implement getters for all new properties
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @speth!
This is a new attempt to integrate the work originally done in #575 and updated in #1319, which enables solving the DAE system that arises when modeling a plug flow reactor with surface chemistry.
Changes proposed in this pull request
DAE_Solver
class (nowIdasIntegrator
)FlowReactor
class1D_pfr_surfchem
surf_pfr.py
to use theFlowReactor
class instead of a chain ofIdealGasReactor
s integrated to steady-state (the previous version is still available assurf_pfr_chain.py
If applicable, provide an example illustrating new features this pull request is introducing
See the two new examples referenced above.
Checklist
scons build
&scons test
) and unit tests address code coverage