Skip to content

Add write data buffer#1612

Merged
BenjaminRodenberg merged 108 commits intoprecice:developfrom
BenjaminRodenberg:add-write-data-buffer
May 15, 2023
Merged

Add write data buffer#1612
BenjaminRodenberg merged 108 commits intoprecice:developfrom
BenjaminRodenberg:add-write-data-buffer

Conversation

@BenjaminRodenberg
Copy link
Copy Markdown
Contributor

@BenjaminRodenberg BenjaminRodenberg commented Apr 3, 2023 •
edited
Loading

Main changes of this PR

Moves buffer, where write data is stored from mesh::Data into WriteDataContext.

Motivation and additional information

Since the ReadDataContext provides read data through a waveform that is stored inside of the ReadDataContext a similar implementation makes sense for WriteDataContext: Store data in a buffer and persist the data in a Storage in the WriteDataContext. This would also simplify mesh:Data by removing the values from mesh:Data.

Author's checklist

  • I used the pre-commit hook to prevent dirty commits and used pre-commit run --all to format old commits.
  • I added a changelog file with make changelog if there are user-observable changes since the last release.
  • I added a test to cover the proposed changes in our test suite.
  • For breaking changes: I documented the changes in the appropriate porting guide.
  • I sticked to C++17 features.
  • I sticked to CMake version 3.16.3.
  • I squashed / am about to squash all commits that should be seen as one.

Reviewers' checklist

  • Does the changelog entry make sense? Is it formatted correctly?
  • Do you understand the code changes?

@BenjaminRodenberg BenjaminRodenberg added this to the Version 3.0.0 milestone Apr 3, 2023
@BenjaminRodenberg BenjaminRodenberg self-assigned this Apr 3, 2023
uekerman
uekerman reviewed Apr 8, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@uekerman uekerman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think such refactoring makes a lot of sense

Comment thread src/time/Sample.hpp Outdated
@BenjaminRodenberg BenjaminRodenberg mentioned this pull request Apr 17, 2023
Closed
13 tasks
@BenjaminRodenberg
Use Stample datastructure.
6f0580e 
* Introduced datastructure in https://github.com/BenjaminRodenberg/precice/tree/move-coupling-data-storage-into-mesh-data
@BenjaminRodenberg
Merge branch 'develop' into add-write-data-buffer
49722bb
@BenjaminRodenberg
Merge branch 'develop' into add-write-data-buffer
2f96b93
@BenjaminRodenberg
Merge branch 'develop' into add-write-data-buffer
13ff18c
@BenjaminRodenberg
Merge branch 'develop' into add-write-data-buffer
bb85e6f
@BenjaminRodenberg
Merge branch 'develop' into add-write-data-buffer
d96170f
@BenjaminRodenberg
Store buffered data from buffer into values (preliminary solution).
4cfa7bc
@BenjaminRodenberg
Store values in storage as samples.
58e7f43
@BenjaminRodenberg
Consequently work with Samples instead of values.
8c096ab
@BenjaminRodenberg
Use context instead of meshData, if possible.
93fa080
@BenjaminRodenberg
Make different write gradient calls more consistent.
838c028
@BenjaminRodenberg
Use context instead of meshData and add ;
0f30882
@BenjaminRodenberg
Use vector data structure and consistent loops.
b046221
@BenjaminRodenberg
Move loops inside data context.
3b7e11e
@BenjaminRodenberg
Write data into buffer.
1ad9ff2
@BenjaminRodenberg
Make loops of write data functions consistent.
fccc046
@BenjaminRodenberg
Move loops inside data context.
a9e5409
@BenjaminRodenberg
Move write data implementation inside WriteDataContext.
d25d83b
@BenjaminRodenberg
Move read data loop inside ReadDataContext.
e104857
@BenjaminRodenberg BenjaminRodenberg mentioned this pull request Apr 27, 2023
Merged
9 tasks
BenjaminRodenberg
BenjaminRodenberg commented May 13, 2023
View reviewed changes
Comment thread src/cplscheme/CompositionalCouplingScheme.cpp
bool isAtWindowEnd = false;
for (const auto scheme : schemesToRun()) {
scheme->addComputedTime(timeToAdd);
isAtWindowEnd |= scheme->addComputedTime(timeToAdd); // @todo should be &= ?
Copy link
Copy Markdown
Contributor Author

@BenjaminRodenberg BenjaminRodenberg May 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fsimonis @uekerman : Any opinion on this? I'm a bit puzzled, because I would expect &=, but with &= I got failures of intergration tests in #1520. In this PR, it does not matter (so we are probably missing a test).

Copy link
Copy Markdown
Member

@uekerman uekerman May 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that we anyway don't support composition of coupling schemes with different time windows. Correct @fsimonis ?
Could then also become an assertion here. All true or all false.

@BenjaminRodenberg BenjaminRodenberg mentioned this pull request May 14, 2023
Open
uekerman
uekerman requested changes May 15, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@uekerman uekerman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Final tweaking.

Do we have an action unit test with subcycling? A single one could be enough already.

Comment thread src/action/PythonAction.cpp Outdated
Comment thread src/action/ScaleByAreaAction.cpp
Comment thread src/action/SummationAction.cpp Outdated
Comment thread src/cplscheme/BaseCouplingScheme.cpp Outdated
Comment thread src/cplscheme/BaseCouplingScheme.cpp
Comment thread src/cplscheme/CompositionalCouplingScheme.cpp
bool isAtWindowEnd = false;
for (const auto scheme : schemesToRun()) {
scheme->addComputedTime(timeToAdd);
isAtWindowEnd |= scheme->addComputedTime(timeToAdd); // @todo should be &= ?
Copy link
Copy Markdown
Member

@uekerman uekerman May 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that we anyway don't support composition of coupling schemes with different time windows. Correct @fsimonis ?
Could then also become an assertion here. All true or all false.

uekerman
uekerman approved these changes May 15, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@uekerman uekerman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Final favor: Please improve error messages 🙏

@BenjaminRodenberg BenjaminRodenberg merged commit bc0b847 into precice:develop May 15, 2023
@BenjaminRodenberg BenjaminRodenberg deleted the add-write-data-buffer branch May 16, 2023 11:12
BenjaminRodenberg added a commit that referenced this pull request May 20, 2023
@BenjaminRodenberg
Add integration test for first order extrapolation + waveform.
f85c010 
* Reproduces bug likely introduced via #1612
kanishkbh
kanishkbh reviewed May 20, 2023
View reviewed changes
Comment thread src/precice/impl/WriteDataContext.cpp

void WriteDataContext::writeValuesIntoDataBuffer(::precice::span<const VertexID> vertices, ::precice::span<const double> values)
{
PRECICE_ASSERT(vertices.size() * getDataDimensions() == values.size());
Copy link
Copy Markdown
Contributor

@kanishkbh kanishkbh May 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assertion is also checked in ParticipantImp::writeData, looks like it can be removed from there

kanishkbh added a commit to kanishkbh/precice that referenced this pull request May 20, 2023
@kanishkbh
Make compatible with "Add write data buffer (precice#1612) "
3fac740
@BenjaminRodenberg BenjaminRodenberg mentioned this pull request May 27, 2023
Merged
11 tasks
kanishkbh added a commit to kanishkbh/precice that referenced this pull request Jun 1, 2023
@kanishkbh
Make compatible with "Add write data buffer (precice#1612) "
aa51a0d
kanishkbh added a commit to kanishkbh/precice that referenced this pull request Jul 6, 2023
@kanishkbh
Make compatible with "Add write data buffer (precice#1612) "
78ccd7f
kanishkbh added a commit to kanishkbh/precice that referenced this pull request Jul 12, 2023
@kanishkbh
Make compatible with "Add write data buffer (precice#1612) "
b1c8cf1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fsimonis fsimonis fsimonis left review comments

@uekerman uekerman uekerman approved these changes

+1 more reviewer

@kanishkbh kanishkbh kanishkbh left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@BenjaminRodenberg BenjaminRodenberg

Labels

maintainability Working on this will make our lives easier in the long run as preCICE gets easier to maintain.

Projects

None yet

Milestone

Version 3.0.0

Development

Successfully merging this pull request may close these issues.

Use data structure similar to time::Storage in mesh::Data::_values

4 participants

@BenjaminRodenberg @uekerman @fsimonis @kanishkbh