Codecov Report

❌ Patch coverage is 74.26471% with 70 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.58%. Comparing base (99550d0) to head (3d6ff95).
⚠️ Report is 40 commits behind head on main.

@@            Coverage Diff             @@
##             main    #1921      +/-   ##
==========================================
- Coverage   75.58%   75.58%   -0.01%     
==========================================
  Files         451      451              
  Lines       56362    56550     +188     
  Branches     9299     9331      +32     
==========================================
+ Hits        42602    42744     +142     
- Misses      10616    10648      +32     
- Partials     3144     3158      +14     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

With this implementation now largely working, I have a couple of observations in relation to the discussion in Cantera/enhancements#201:

• We cannot rely on always cloning the Solution objects used in a reactor network. The most severe issue with this is that CustomRate objects implemented as Python lambdas cannot be serialized. This is currently the reason the custom_reactions.py example fails to run.
• We cannot make cloning Solution objects the default behavior in this Cantera version. Based on the changes to various test cases and examples, it is clear that there is a fair amount of existing code that relies on the incidental sharing of Solution states. While identifying these cases in the test suite is relatively easy, it is not so easy elsewhere and would be a significant breaking change.

As such, I think the next steps for this PR are:

• Implement an additional clone argument to the Reactor and ReactorSurface constructors, which will default to None, In Cantera 3.2, if no value for this argument is provided, the Solution will not be cloned and a warning will be issued. In Cantera 3.3, we can make clone=True the default, while clone=False will still be available for special cases.
• Add a check during ReactorNet::initialize to identify whether any Solution objects are being used multiple times within the network. In Cantera 3.2, doing so will result in a warning, which can then be converted to an error in Cantera 3.3.

@speth ... thanks for your work on this, as well as the thoughts for the roadmap, which looks 👍.

I was somewhat surprised that CustomRate objects with Python lambdas cause failures, which is not among the consequences that I would have expected. On second thought, I assume that it has to do with those not being part of the YAML serialization, and thus cannot be cloned? If this were the case, I'd suggest simply throwing an error if there's an attempt to clone a mechanism that includes CustomRate (I have not started a review, so it may already be there). I assume this is purely limited to Python lambdas, as C++ has no way of ensuring that Python objects persist in memory? I.e., CustomRate with Func1 objects should work?

Beyond, it appears that #1923 fixes other sample tests that are currently failing.

I was somewhat surprised that CustomRate objects with Python lambdas cause failures, which is not among the consequences that I would have expected. On second thought, I assume that it has to do with those not being part of the YAML serialization, and thus cannot be cloned?

I didn't anticipate this either, but it's pretty clear in retrospect. Indeed, the issue is that such lambdas cannot be serialized.

If this were the case, I'd suggest simply throwing an error if there's an attempt to clone a mechanism that includes CustomRate (I have not started a review, so it may already be there). I assume this is purely limited to Python lambdas, as C++ has no way of ensuring that Python objects persist in memory? I.e., CustomRate with Func1 objects should work?

This affects all CustomRate objects, including ones built on Func1, since the CustomFunc1Rate::getParameters method is not implemented (or rather, explicitly raises a NotImplementedError). In principle, we could implement serialization of Func1-based rates, but it's not strictly necessary. The fallback is for the user to instantiate independent Solution objects and call the Reactor constructor with the clone=False option.

Likewise, ExtensibleRate objects that don't implement the set_parameters / get_parameters pair will require this alternative approach, as will any other user-defined model that doesn't fully implement serialization.

The "next steps" I mentioned before are now completed, but I'm currently struggling with how to get this to play nice with the generated CLib. I added a new method, shared_ptr<Solution> ReactorBase::solution() for accessing the Solution object used by a Reactor. Depending on whether a clone was created or not, this is either the new cloned object or just the same object that was passed into the constructor. In the first case, I understand why CLib is struggling -- that cloned Solution isn't already in the cabinet, but I'm not sure what the right way to update the code generation for that is. In the second case, this accessor also doesn't seem to be working, and I can't figure out why.

Thanks, @speth for further context.

[...] In principle, we could implement serialization of Func1-based rates, but it's not strictly necessary. The fallback is for the user to instantiate independent Solution objects and call the Reactor constructor with the clone=False option.
Likewise, ExtensibleRate objects that don't implement the set_parameters / get_parameters pair will require this alternative approach, as will any other user-defined model that doesn't fully implement serialization.

It should be pretty straight-forward to come up with Func1 serialization so things work out of the box. For ExtensibleRate, it's obviously up to the users. This clearly goes beyond this PR.

The "next steps" I mentioned before are now completed, but I'm currently struggling with how to get this to play nice with the generated CLib. I added a new method, shared_ptr<Solution> ReactorBase::solution() for accessing the Solution object used by a Reactor. Depending on whether a clone was created or not, this is either the new cloned object or just the same object that was passed into the constructor. In the first case, I understand why CLib is struggling -- that cloned Solution isn't already in the cabinet, but I'm not sure what the right way to update the code generation for that is. In the second case, this accessor also doesn't seem to be working, and I can't figure out why.

Hmm. This makes sense - it wasn't straightforward to get the initial accessors to work properly (most relevant example: sol_adjacent); I managed to address this with the uses field. While the Python code should be relatively easy to follow (I did go back and revised), the conditionals in Jinja are quite difficult to make readable (this may be improved by Cantera/enhancements#232).

At the same time, the existing logic may be sufficient to handle this situation using your new shared_ptr<Solution> ReactorBase::solution() method. The cabinet entries should be created during constructor calls for Reactor and Domain1D objects. Fortunately, the Jinja logic for constructors is among the more straightforward/better commented ones:

clib-constructor: |-
  ## CLib constructor template: instantiates new object
  // constructor: {{ cxx_wraps }}
  try {
      {% for line in lines %}
      ## add lines used for CLib/C++ variable crosswalk
      {{ line }}
      {% endfor %}
      {% if shared %}
      ## instantiated object manages associated objects (see: sol3_newSolution)
      ## storage of associated objects requires id (handle) of new object
      auto obj = {{ cxx_name }}({{ ', '.join(cxx_args) }});
      int id = {{ base }}Cabinet::add(obj);
      {% for typ, getter in shared %}
      ## add associated objects (see: sol3_newSolution, sol3_adjacent)
      if (obj->{{ getter }}()) {
          {{ typ }}Cabinet::add(obj->{{ getter }}(), id);
      }
      {% endfor %}
      return id;
      {% else %}{# not shared #}
      ## instantiated object has no associated objects; no id is needed
      return {{ base }}Cabinet::add({{ cxx_name }}({{ ', '.join(cxx_args) }}));
      {% endif %}{# shared #}
  } catch (...) {
      return handleAllExceptions({{ error[0] }}, {{ error[1] }});
  }

Specifically, you'd need to get the Solution object clones into the following portion via the shared argument:

      {% for typ, getter in shared %}
      ## add associated objects (see: sol3_newSolution, sol3_adjacent)
      if (obj->{{ getter }}()) {
          {{ typ }}Cabinet::add(obj->{{ getter }}(), id);
      }
      {% endfor %}

As the cloned Solution is a managed object, the CLib destructors need to use the uses field as well.

As an aside, I noticed that there are some stray sol3_* references left in the ## Jinja comment blocks 😢 (should be sol_*).

PS: In a nutshell, you may need to update ctreactor_auto.yaml as follows:

- name: new
  wraps: newReactorBase
  uses: [solution]
  [...]
- name: del
  uses: [solution]

I haven't tested it, though.

Hmm. This makes sense - it wasn't straightforward to get the initial accessors to work properly (most relevant example: sol_adjacent); I managed to address this with the uses field. While the Python code should be relatively easy to follow (I did go back and revised), the conditionals in Jinja are quite difficult to make readable (this may be improved by Cantera/enhancements#232).

Thanks for pointing me to the uses field, and the template that's use for the newReactorBase method. This seems to get some of the necessary behavior right with minimal effort. However, it's not quite there for both cases of either cloning or not cloning the Solution:

• In the case where the Solution object is not cloned, this results in an additional reference to the Solution being added to the cabinet, with a new ID. This new Solution ID then cannot be used to obtain the thermo member using sol_thermo.
• In the case where the Solution is cloned, we get a new ID which is correctly retrieved via reactor_solution. However, the components of this Solution like the thermo object are not themselves being added to the corresponding cabinets, again causing sol_thermo to fail, and I'm not sure where that logic should live.

I think I don't fully understand what the whole "parent" logic within Cabinet does, which is maybe contributing to my confusion here. The current situation is a bit complicated by the fact in one case, the Solution should be "owned" by the Reactor and (presumably) deleted when the Reactor is deleted, but in the other case the Solution should outlive the Reactor.

@ischoegl, thanks for the help sorting out the new clib aspect of this. I think this is ready for review now.

The complaint from Codecov is at least partly incorrect, due to it miscounting coverage of the lambda functions in ReactorFactory, and much of the other uncovered code is in deprecated methods that are only needed for transitional purposes.

P.S.: Thinking about this some more, I'm curious about the rationale for not using Solution clones in the future: do you see any applications where people would like to use a single copy? I'm honestly leaning towards just using clones (i.e., no shared objects). Apart from Python users, whom we can warn with the None workaround, anyone using implicit sharing as a 'feature' is in for a surprise once the default changes to 'clone'. I'd rather just rip that band-aid off and avoid introducing the additional parameter ...

P.S.: Thinking about this some more, I'm curious about the rationale for not using Solution clones in the future: do you see any applications where people would like to use a single copy? I'm honestly leaning towards just using clones (i.e., no shared objects).

Yes, I believe there are cases where not cloning the Solution object is preferable. Besides the relatively small performance benefit, the main reason is to handle cases where the Solution is not (correctly) serializable. There are currently several places where this may happen:

• Any use of CustomRate, where the rate is defined as a Python lambda function
• Any use of ExtensibleRate where the user has not gotten as far as implementing the get_parameters and set_parameters methods to allow their rate class to be serialized. While users should be able to implement these methods, I think it is an unnecessary burden in some cases, and it would significantly increase the complexity of just trying out a new rate parameterization to see if it's useful before going to the effort of implementing these additional utility methods.
• Any in-development thermodynamic or kinetics/reaction models implemented in C++, where again, it should be possible to start with implementing the methods that describe the physics rather than having to have all of the utility methods working right from the start.

Apart from Python users, whom we can warn with the None workaround, anyone using implicit sharing as a 'feature' is in for a surprise once the default changes to 'clone'. I'd rather just rip that band-aid off and avoid introducing the additional parameter ...

What existing (as of Cantera 3.1) use case doesn't provide a warning or notification about this change?

• make_shared<Reactor>(soln, name) issues a deprecation warning about "clone argument not specified and notes the pending change in behavior"
• newReactor("Reactor", soln, name) issues a deprecation warning about the return type change and redirects the user to newReactor4 where the clone argument is added. I guess the warning here should also mention the change in the cloning behavior, given the path is not removal of this method but to change the existing behavior.
• newReactor4 is "new in Cantera 3.2" and defaults to clone=true
• newReservoir is "new in Cantera 3.2" and defaults to clone=true
• newReactorSurface is "new in Cantera 3.2" and defaults to clone=true

My only remaining concern was to start the deprecation cycle for ReactorBase::contents so we can start moving things. I'm 👍 for this to be a separate PR.

I'm happy to add that to this PR; just haven't had time to work on this in the past week. Please hold off on merging this.

Actually, scratch that. I started in on migrating ReactorBase::contents along with renaming the Python Reactor.thermo and ReactorSurface.kinetics methods to also be contents, and it touches a lot of code, so I'll take that up in a separate PR.

@speth ... in order to handle this consistently going forward, do you believe it to be necessary to extend cloning to Domain1D and SolutionArray?