-
-
Notifications
You must be signed in to change notification settings - Fork 349
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
Delegate reactor implementation to user-provided Python classes #1003
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1003 +/- ##
==========================================
+ Coverage 66.10% 66.30% +0.19%
==========================================
Files 313 315 +2
Lines 44955 45283 +328
Branches 19137 19237 +100
==========================================
+ Hits 29719 30023 +304
+ Misses 12652 12644 -8
- Partials 2584 2616 +32
Continue to review full report at Codecov.
|
53e88af
to
ecd2232
Compare
While I know this is work-in-progress, I was wondering about names … in some of my recent work in the context of reactions/rates, I used |
IMO, this is exactly the point of draft PRs, to address and resolve bigger questions about the implementation 😊 I agree we should try to be consistent. |
We recently gained 8 characters per line!! 😄 ... Joking aside, I was suggesting |
I went through several names while working up this implementation, and I would agree that it's tricky to come up with an ideal name. My rationale for using the term I guess one alternative would be (and no worries about comments on the draft PR - they're entirely welcome, from my end) |
Thanks, @speth. I actually don’t have any concerns about the internal |
I'm concerned that a name like |
That remains a valid point. What about |
@speth ... thanks for your comments. I am responding outside of the thread above as I believe I have some bigger questions about the proposed implementation.
It would be relatively straight forward to create a generic
I don't think that it would be a good idea to thread everything through a single // For functions with the signature void(double*)
virtual void setDelegate(
const std::string& name,
const std::function<void(std::array<size_t, 1>, double*)>& func,
const std::string& when) override
{
if (name == "getState") {
// ... set delegator
} else if (name == "updateState") {
// ... set delegator
} else if (name == "updateSurfaceState") {
// ... set delegator
} else if (name == "getSurfaceInitialConditions") {
// ... set delegator
} else {
throw NotImplementedError("ReactorDelegator::setDelegate",
"For function named '{}' with signature"
" void(array<size_t, 1>, double*)", name);
}
} My main fear here is that this will be hard to maintain long term as what is done is well hidden. An alternative would be to do this explicitly by declaring a dedicated delegateGetState(const DoublePtrFunc& func)
{
// ... set delegator
}
delegateUpdateState(const DoublePtrFunc& func)
{
// ... set delegator
}
delegateUpdateSurfaceState(const DoublePtrFunc& func)
{
// ... set delegator
}
delegateGetSurfaceInitialConditions(const DoublePtrFunc& func)
{
// ... set delegator
} Some of the plumbing would be different (a I honestly believe that an alternate explicit structure would be easier to maintain and could also be directly applied to other Cantera objects without adding too much to the boilerplate. I also believe that the outside interface (e.g. Python API) can be the exact same as what is already proposed. PS: I see how your implementation is motivated by PPS: As yet another alternative, what about setting reg("getState", [](const GenericFunc& func) { m_getState = new DoublePtrFunc(func); }); which then allows to simply call |
I don't have the C++ experience to contribute any code suggestions, but I'm excited for this feature and the capability for custom functionality in Python. I did want to comment on some of the naming discussed in the thread though and offer my thoughts. I also think that I'm not sure if I fully understand how the class would work. Is it a simple reactor for which basic behavior is defined with the option to overload or extend certain components, or is it an empty reactor where certain functions must be defined in order for it to be usable? If it's the former, |
For the factory pattern to be useful, you need to be able to construct different derived-type objects from input that has the same type, e.g. how we construct a
Switching from the relatively small set of I disagree that the use of strings with names that correspond to function names in the C++ interface is in akin to magic numbers. You're going to have some sort of name mapping no matter what when crossing the language interface, and this is no worse than the pattern of using names like
This is an interesting suggestion, and I think you could use this to make the if (name == "initialize) {
m_initialize = makeDelegate(func, when,
[this](double t0) { R::initialize(t0); }
);
} With the introduction of the reg("initialize",
[this](const std::function<void(double)>& func, const std::string& when) {
m_initialize = makeDelegate(func, when,
[this](double t0) { R::initialize(); });
}
); which really doesn't win any awards for readability, and would just be worse for the cases where
The latter behavior is what this feature accomplishes. I like the idea of using the prefix |
ecd2232
to
11c6093
Compare
@ischoegl - I've rebased this so it has the same root as #1130. This PR contains my initial implementation of the core "delegation" feature, while @chinahg's PR introduces an additional change to the way all of the reactor governing equations are written to make it easier to introduce certain modifications to those equations. For review purposes, I think the best thing to do at least initially would be to review my changes in this PR, and limit comments on #1130 to the changes that are unique to that PR. |
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 ... while this is technically still in Draft mode, I left a couple of comments. The external interface looks like a good start (there may be a way of making it a little more user-friendly, but it would probably have to build on something similar to what is proposed here).
My main question is whether the 'glue' between generic Python functions and C++ can be written in a more generic fashion. The cascading if
-else
are hard to read, and the new PyFuncInfo
look like they would be terrific to have outside of delegated objects.
For the sake of exploration, I tried modifying this to use individual member functions to set delegates, for functions of one particular signature ( Using the } else if (name == "updateState") {
m_updateState = makeDelegate<1>(func,
[this]() {
return std::array<size_t, 1>{R::neq()};
},
when,
[this](double* y) {
R::updateState(y);
}
);
} and a single line added to the Python 'update_state': ('updateState', 'void(double*)'), In contrast, with individual delegate setters, the setter in virtual void setUpdateState(const std::function<void(std::array<size_t, 1>, double*)>& func,
const std::string& when) override
{
m_updateState = makeDelegate<1>(func,
[this]() {
return std::array<size_t, 1>{R::neq()};
},
when,
[this](double* y) {
R::updateState(y);
}
);
} and also requires a corresponding declaration in the newly-required virtual void setUpdateState(const std::function<void(std::array<size_t, 1>, double*)>& func,
const std::string& when) = 0; On the Python side, this method needs to be declared to Cython as part of the void setUpdateState(function[void(size_array1, double*)], string&) except +translate_exception and a new if/else tree is needed to check for delegates in for when in ['before', 'after', 'replace']:
cxx_when = stringify(when)
if hasattr(self, f"{when}_update_state"):
method = getattr(self, f"{when}_update_state")
delegator.setUpdateState(pyOverride(<PyObject*>method, callback_v_dp), cxx_when) If there was something else you had in mind, I'd be interested to hear it, but I don't think this is solving any problems. |
@speth ... thank you for looking into this. I believe I have an idea that I'd like to try (i.e. I'm planning to actually implement something, but I won't have time until the end of the week). That said, the main question is about |
@speth ... I looked at this in some more detail, and agree that while generic functions and delegators have some common traits, they really just apply for the case Beyond, I agree that your alternative implementation doesn't improve things, as the issue is that call-back functions need to be built somehow. One thing that I tried (briefly) was to use C++ function pointers. For example: typedef void(R::*voidFromDoubleMethod)(double);
template <voidFromDoubleMethod method>
std::function<void(double)> voidFromDoubleFunc(
const std::function<void(double)>& func,
const std::string& when)
{
return makeDelegate(func, when, [this](double value) { (this->*method)(value); });
}
virtual void setDelegate(const std::string& name,
const std::function<void(double)>& func,
const std::string& when) override
{
if (name == "initialize") {
m_initialize = voidFromDoubleFunc<&R::initialize>(func, when);
[...]
} The main idea is to move the hard-to-parse stuff to a template, and use self-explanatory templated functions to do the work. In theory, this should cut down boilerplate significantly. In practice, this compiles fine, but segfaults, although some smaller toy problems worked without issues. So this may be somewhat fragile. Another - somewhat different - idea I stumbled across uses void noVoidFromValue(double value) {}
std::map<std::string, int> timing { {"before", 0}, {"replace", 1}, {"after", 2}};
virtual void initialize(double t0) override{
m_initialize[0](t0); // set initially to std::bind(noVoidFromValue, _1);`
m_initialize[1](t0); // set initially to std::bind(R::initalize, _1);
m_initialize[2](t0); // set initially to std::bind(noVoidFromValue, _1);
} where virtual void setDelegate(const std::string& name,
const std::function<void(double)>& func,
const std::string& when) override
{
if (name == "initialize") {
m_initialize[timing[when]] = std::bind(func, _1);
} else if (name == "evalWalls") {
m_evalWalls[timing[when]] = std::bind(func, _1);
} else {
throw NotImplementedError("ReactorDelegator::setDelegate",
"For function named '{}' with signature void(double)", name);
}
} I'm pretty sure this can be simplified further. One nice thing about this implementation is that it should be easy to follow. I'm aware of bind having a modest (?) performance penalty, but I wonder whether this would be a bottleneck here. It's probably possible to implement the same with lambda functions ... PS: I obviously picked a really simple example here (no return arguments, etc.). In this case, the resulting code simplifies quite a bit, but it will get more involved for other |
@speth ... I ended up running a more comprehensive trial where I replaced The implementation is the last commit on https://github.com/ischoegl/cantera/tree/delegating-reactor (4beffaa) ... if fully implemented, this would avoid the templates introduced by |
Thanks, @ischoegl, this has been helpful for generating some interesting ideas. One interesting idea that you have in the implementation that you linked to, but didn't mention in your description is that you've eliminated the need for a separate function object to get array sizes by just calculating those in the corresponding overridden method of the Reactor class, e.g. virtual void eval(double t, double* ydot) override {
std::array<size_t, 1> neq = {R::neq()};
....
} I had given some thought early on to just calling a fixed chain of before/replace/after methods. I'm glad to see you got this working at least for methods that have no return type. The case where I think this gets more complex is for methods that do have a return value and still supporting the before/after options, which is why I went with the structure that exists now. I did have an idea based on your mention of function pointers, though, which led me to a way that does actually eliminate the |
🎉 @speth ... I really like this idea which I honestly believe is a great concept and a huge simplification of the boilerplate! As a friendly amendment, I further believe what you pointed out about my trial can be combined here to further simplify things as: install("updateState", &m_updateState,
[this](std::array<size_t, 1> neq, double* y) { R::updateState(y); }
); and virtual void updateState(double* y) override {
std::array<size_t, 1> neq = {R::neq()};
m_updateState(neq, y);
} I didn't test this, but you probably get the idea ... the PS: one thing that comes to mind is that at some point there may be a need to call one function before and another function after? I think that if all functions (original and delegated) share the same signature, the function calls can be compounded, so there may be only a need for a single PPS:
I had this mostly coded, but stumbled across some undocumented string parameters and just pushed what was working as it already served the purpose. The idea would have been to move the decisions into the local |
Yes, that's exactly what I was implying, and would have the simplification of
I'm not sure whether there's anything to be buffered here. For one, the value returned by
Yes, this idea of using the current function and wrapping it with the incoming "before" or "after" function had crossed my mind previously. I had kind of shied away from it because it felt strange to lose a way to directly call the original version of the function, but as long as no one wants to keep changing the delegated methods on an existing object, it shouldn't really matter. I couldn't come up with a case where wanting to do both "before" and "after" seemed like the best option, but there might be such cases somewhere, and I think doing this would also enable those without any additional hassle. |
7ef1856
to
4744871
Compare
When adding reactions, the check for whether or not surface sites are conserved needs to occur after the check for whether all species in the reaction exist, since the number of sites isn't known for an undefined species.
This signature makes it possible to write delegates for this function that change the gas phase species production rates
This is analogous to the automatic synchronization done when accessing Reactor.thermo.
This prevents the creation of circular references that aren't traversable by the Python garbage collector and therefore can never be reclaimed.
28d2921
to
3d813ed
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.
Thank you, @speth! This looks good to me.
Changes proposed in this pull request
Delegator
class which can be used to delegate methods of a Cantera object to user-provided methodsExtensibleReactor
class in Python, which allows users to write a Python class that augments or replaces behavior of theReactor
class. Similar lightweight classes are added for the other reactor types, namelyExtensibleIdealGasReactor
,ExtensibleConstPressureReactor
, andExtensibleIdealGasConstPressureReactor
.Reactor
evaluation to meet a couple of needsevalEqs
toeval
and remove they
andparams
arrays that shouldn't be used withineval
for simplicity and to prevent some potential for errorsevalSurfaces
method to eliminate the return value and to add the bulk phase species production rates as an additional outputeval
andevalSurfaces
to split theydot
argument into "left hand" and "right hand" sides of the ODE, to make it easier for users to augment the existing implementationsReactor
objectRemaining To-do items:
std::function<...>*
to remove need for specializingsetDelegate
in derived classesDelegatedReactor
class toExtensibleReactor
Delegator
and Cython callback functionsAn example of a user-provided class is given in the new example
custom2.py
:cantera/interfaces/cython/cantera/examples/reactors/custom2.py
Lines 23 to 64 in eed11d7
If applicable, fill in the issue number this pull request is fixing
This is the first part of implementing the capabilities described in Cantera/enhancements#79. The intention is to exercise this capability first for the
Reactor
class before expanding its use to other areas like reactions and thermo models.Checklist
scons build
&scons test
) and unit tests address code coverage