Shift ownership of Waveform into configuration#1133
Shift ownership of Waveform into configuration#1133BenjaminRodenberg wants to merge 4 commits intoprecice:developfrom
Conversation
…nd initialization.
BenjaminRodenberg
added
the
maintainability
uekerman
left a comment
uekerman
left a comment
There was a problem hiding this comment.
Goes in the right direction 👍
We could already introduce a WaveformConfiguration.cpp/TimeConfiguration.cpp here. This would be mostly empty, but provides some structure for #1029. What's you opinion?
I would only add it once we know what we want to configure. If we realize that there is not so much to configure, we could also keep it in the coupling scheme configuration.
Let's find a better name for setupDataMatrices.
| const mesh::PtrData & data, | ||
| const time::PtrWaveform &ptrWaveform, |
There was a problem hiding this comment.
Why do we have both data and waveform here? Shouldn't the one replace the other?
There was a problem hiding this comment.
I don't think that this is easily possible. The Waveform is currently just a buffer for values to be used for interpolation and extrapolation. The Data also stores the id and the name of the data - and of course the _values itself.
I guess we could replace Data::_values with a pointer to the data stored in the waveform. Then the Waveform will become our only store for values. But this is a big step, because Data::_values are used and updated almost everywhere (also outside of the time or cplscheme package). I am also not sure whether this is a good idea, because then all packages have to interact with the time package and the Waveform.
Currently we are copying Data::_values into the Waveform on demand. Only the cplscheme packages accesses the waveform. Other packages do not have to worry about the waveform at all, because it is wrapped by the cplscheme. Storing data happens here:
precice/src/cplscheme/BaseCouplingScheme.cpp
Line 265 in b784b38
There was a problem hiding this comment.
BenjaminRodenberg#4 actually goes some steps into this direction, but entirely removing the PtrData is, as explained above, a big step.
| time::PtrWaveform ptrWaveform(new time::Waveform(_config.extrapolationOrder)); | ||
| // @todo store all waveforms in _waveforms? See acceleration config. | ||
| scheme.addDataToSend(exchange.data, ptrWaveform, exchange.mesh, requiresInitialization); |
There was a problem hiding this comment.
Couldn't we create the waveforms closer to the time-stepping part of the config? At some point, we will also get configuration options there. Here, it feels a bit misplaced.
There was a problem hiding this comment.
I would rather move this at a later point in time, if necessary. Currently adding the waveform at a different point requires us to add more functions to the CouplingScheme (addWaveformToReceive, addWaveformToSend) and I don't see a clear benefit.
Right now adding everything with a single function enforces the rule "each data has a waveform". This rule might be removed at a later point in time, but currently we are following this rule (because also extrapolation is applied globally to all data).
There was a problem hiding this comment.
I started drafting the shift of the ownership of the Waveform into Data. This is also a possible way to go, but still needs some refinement. See BenjaminRodenberg#4.
However, I think it automatically solves this problem.
Renamed it in eb035ff to |
|
I think we can close this PR when #1145 is merged:
Let's keep this PR open until #1145 is merged. Then I will check again and see whether this PR still contains something we want to keep. |
|
Closing the PR, because it is outdated. Refer to #1145. |
2 participants
Main changes of this PR
Waveforms are now created during configuration. The resulting object is owned by the configuration class. Other classes only receive a pointer to this object.
This requires us to split creation and initialization of the Waveform object, because
valuesSizeis not known during configuration.Motivation and additional information
Main motivation is this observation: https://github.com/BenjaminRodenberg/precice/pull/1/files#r745679418
Collecting waveforms at a central place should help us to avoid duplicates and get a better overview.
Author's checklist
docs/changelog/if there are noteworthy changes.tools/formatting/check-formatand everything is formatted correctly.Reviewers' checklist