Refactor H isotope charge exchange to use the new reactions framework.#398
Refactor H isotope charge exchange to use the new reactions framework.#398oparry-ukaea merged 111 commits intoboutproject:masterfrom
Conversation
…mjuelHydIsotopeReaction.
| * @return std::vector<std::string> vector of keys | ||
| */ | ||
| template <typename T> | ||
| static inline std::vector<std::string> str_keys(const std::map<std::string, T>& map) { |
There was a problem hiding this comment.
C++20 has std::views::keys -- not for this PR, but worth thinking if we can move to C++20 @bendudson ?
… functions in Reaction subclasses.
|
Tests all passing (after loosening the relative tolerance used in 2D-production slightly). |
mikekryjak
left a comment
mikekryjak
left a comment
There was a problem hiding this comment.
@oparry-ukaea thanks for the changes. Could you add a simple, high level executive summary at the start of the Reactions section in the dev manual that describes how the different classes interact? It may make it a bit easier to understand the flow.
Co-authored-by: mikekryjak <62797494+mikekryjak@users.noreply.github.com>
Added something - I'm reluctant to do anything more detailed at the moment because this will change again once the data-decoupling PR is finished... |
Perfect - this is good. The PR is good as far as I'm concerned. @bendudson can you review? |
dschwoerer
left a comment
dschwoerer
left a comment
There was a problem hiding this comment.
There are a few things that need to be done to make it more FCI friendly. It also could be cleaned up a tiny bit more.
| the stoichiometry matrix would be: | ||
|
|
||
| .. _fig-stoichiometry_matrix_eg: | ||
| .. figure:: figs/stoichiometry_matrix_eg.* |
There was a problem hiding this comment.
Could you convert this to a math block?
There was a problem hiding this comment.
I could, but it's a bit of a pain. I can't find the latex I originally used to generate this, but I remember it being fiddly to force it to align the column labels the way I wanted...
dschwoerer
left a comment
dschwoerer
left a comment
There was a problem hiding this comment.
I think this can be merged as is ...
| std::string toString(ReactionDiagnosticType diag_type); | ||
|
|
||
| static const std::map<ReactionDiagnosticType, std::string> state_labels = { | ||
| static const std::map<ReactionDiagnosticType, std::string> sp_data_keys = { |
There was a problem hiding this comment.
Yes. Happy to change it if you think something else is clearer!
dschwoerer
left a comment
dschwoerer
left a comment
There was a problem hiding this comment.
I nearly forgot, could you ensure CI is passing?
Can you push to this fork?
Co-authored-by: David Bold <dave@ipp.mpg.de>
Checks all passing: |
mikekryjak
left a comment
mikekryjak
left a comment
There was a problem hiding this comment.
Looks good as far as I'm concerned. Thanks @oparry-ukaea!
bendudson
left a comment
bendudson
left a comment
There was a problem hiding this comment.
Thanks @oparry-ukaea ! This is quite monumental.
|
@dschwoerer - Happy for us to merge this? |
oparry-ukaea
dismissed
dschwoerer’s stale review
@dschwoerer has confirmed offline that he's happy - I'll dismiss his review so that we can expedite this!
HCX refactor PR (#398) introduced tiny changes which were merged into master between me shortening the tests and merging the PR
5 participants
Some implementation details:
to existing (n,T). Placeholder for future (E,T) fits.
decides which to use at runtime. This allows subclasses to (for instance) read the
type from a json file.
default splitting of reactant momentum and energy between products.
the outgoing neutral and vice-versa.
reactions (e.g. h + h+ -> h+ + h) where pop changes are zero.
reordering, compiler optimisations.
ToDo:
(moved all but frictional heating)