Reservoir coupling: Send more data from slaves to master#6567
Conversation
|
jenkins build this please |
0aa16e8 to
48c4bce
Compare
|
jenkins build this please |
48c4bce to
8702258
Compare
|
jenkins build this please |
|
jenkins build this serial please |
8702258 to
f8fab7b
Compare
The master needs more data than the currently sent slave group potentials: - surface production rates - reservoir voidage rates - reinjection rates - surface injection rate - reservoir injection rate
|
How many types as scalar are we actually supporting? If it is just a few the we should not move the all definitions to the header files. Instead we should rather keep them in the *.cpp and should explicitly instantiate the template classes for the types needed. This will make this PR smaller and will reduce compile time. (If we miss the instantiation for certain types, it will produce linker errors, though). Please comment whether you think that this is a good idea and you will do it. |
@blattms Thanks for the comment. We are supporting the same two types as elsewhere:
I believe the PR already follows the pattern you're suggesting: All template implementations remain in
The header files contain only declarations, not definitions. |
blattms
left a comment
There was a problem hiding this comment.
I think this looks good.
A few comments:
We have introduced another template parameter for a lot of classes. Sometimes it seems this done only to store reference to the master and used only to call functions that might not eve depend on the type of scalar. Maybe we are able to remove some of the introduced template parameter later. Possible ways are
- to call a method on an object one can use a functor that we store (instead of a reference to an object
- There could be base classes without a template parameter that provide methods that do not need it
- One could you double internally and only convert to Scalar when needed. this would cost a bit of storage and make messages bigger, though.
Tnis might be more work than anticipated by me, though. Hence let's do this later.
I would a appreciate a bit more doxygen comments, e.g. describing what the classes arre for and also more complicated methods.
f8fab7b to
20244bd
Compare
|
jenkins build this please |
|
jenkins build this serial please |
|
@blattms Thanks for the detailed review. I have added 12 commits to address the comments. |
blattms
left a comment
There was a problem hiding this comment.
Thanks a lot for all the changes.
Splitting it into one commit per change made reviewing really easy. Highly appreciated.
Note: Builds on #6535 which should be merged first.
The master needs more data than the currently sent slave group potentials. I.e.
This is in preparation for #6347 which should be rebased once this is merged.