Skip to content
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

Synchronization of ReactionRate objects: Reaction vs MultiRate #1211

Open
ischoegl opened this issue Mar 6, 2022 · 5 comments
Open

Synchronization of ReactionRate objects: Reaction vs MultiRate #1211

ischoegl opened this issue Mar 6, 2022 · 5 comments
Labels

Comments

@ischoegl
Copy link
Member

ischoegl commented Mar 6, 2022

Problem description

Both 'legacy' and 'new' frameworks copy reaction rate objects into contiguous arrays of objects that are evaluated by Kinetics specializations (as all objects are in consecutive memory, access is efficient). As a result, it is necessary to use a modifyReaction route to change parameters, etc.. In addition, it is also not possible to query internal states of the rate object from a Reaction that was used to originally create the rate.

Behavior

The new framework being introduced in 2.6 allows for a more uniform access to Reaction and ReactionRate objects, but currently does not synchronize parameters and internal states once a Reaction is added to a Kinetics object. A user attempting to query any information will always access internal states of the original copy owned by Reaction, and is unable to access the copy used for evaluation (see, e.g. discussion in #1181).

System information

  • Cantera version: 2.6.0a4
  • OS: any
  • Python/MATLAB/other software versions: N/A

Additional context

Potential solutions.

  1. Use a single copy of a ReactionRate object
  2. Keep copies synchronized (see proof-of-concept in Proof-of-concept: Modify reaction rates in memory #1051)

As long as performance is comparable, solution (1) is preferable. Notably, the Python API requires shared_ptrs for access, which may create additional overhead. Also see blog post, which indicates that (2) is likely to outperform (1).

@speth
Copy link
Member

speth commented Mar 13, 2022

Out of curiosity, I just tested the option of sharing ReactionRate objects between Reaction and Kinetics objects (see speth@b8f6132), and found that it had no noticeable performance impact, at least for the comparison that's done by the custom_reactions.py example. I think the reason why using (effectively) a vector<ReactionRate*> is able to have similar performance to using a vector<ReactionRate>, unlike the blog example linked, is because our usage is almost entirely read-only, which will still benefit from caching, while writing to such structures as in that example will incur writes to many distinct locations in main memory.

More broadly, I think there are actually two separate categories of information here where synchronization is a concern. The first is for parameters that define the reaction rate itself, where the question of synchronization is how to get the Kinetics object to see changes to a Reaction's rate object. This is currently handled by the use of modifyReaction. The patch linked above essentially provides this without the need to use modifyReaction, with the exception that a lot of rate parameters don't currently have dedicated setters (e.g. there is no ArrheniusRate::setActivationEnergy method).

The other category is "properties" of the rate that are dependent on the state of the ThermoPhase / Kinetics objects, like the activationEnergy method of interface reaction rates where this can depend on the coverages. This is actually a new situation created by the new reaction handling framework. Previously, such "effective" properties were available directly from the Kinetics object, but those methods were deprecated in #1181 in favor of the methods of the ReactionRate classes. The patch linked above doesn't quite solve the problem for this situation -- the properties accessed via the ReactionRate will only be up-to-date as of the last time reaction rates were evaluated, but won't reflect any changes to the thermodynamic state after that.

@ischoegl
Copy link
Member Author

ischoegl commented Mar 13, 2022

That's really welcome news! One other thing that's different here is that the blog deliberately randomized the memory structure, whereas for MultiRate it is likely that we have adjacent memory blocks. One thing that can be done here is to reserve memory in advance to ensure that this optimization can be used (although I'm not sure how this would be handled).

I overall agree with your assessment about the two separate categories, as well as the fact that the second one was just introduced by #1181. My own thinking was that if it were possible to use vectors of pointers, the original ReactionRate objects that the Reaction owns could be replaced by the copy that was (re-)created by MultiRate. This would be trivially easy if we use std::vector<std::shared_ptr<ReactionRate>>, but may incur some speed penalties beyond what you just saw for the raw pointers.

@ischoegl
Copy link
Member Author

@speth ... are you planning on converting speth@b8f6132 to a PR?

@speth
Copy link
Member

speth commented Mar 15, 2022

I'm not sure it makes sense right now. In light of #1217, it becomes a less-than-complete solution. I think something along the lines of #1051 will be required in any case to get consistent synchronization, and having "just modify Reaction.rate" work some times and not others is worse than having it take no effect unless modifyReaction is called.

@ischoegl
Copy link
Member Author

I agree that things may be somewhat premature and don't think that there is a pressing need. I think it makes sense to defer until after 2.6.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Development

No branches or pull requests

2 participants