Adds storeIteration for initial write data by KyleDavisSA · Pull Request #1036 · precice/precice

KyleDavisSA · 2021-06-23T15:29:16Z

Main changes of this PR

stores the previousIteration after calling writeInitialData.

Motivation and additional information

This PR closes #1034 and #1032

Author's checklist

I added a changelog file with this PR number in docs/changelog/ if there are noteworthy changes. -> Fixes a bug, but the buggy version was never released, so no need to add anything to the changelog.
I ran tools/formatting/check-format and everything is formatted correctly.
I sticked to C++14 features.
I sticked to CMake version 3.10.

Reviewers' checklist

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

MakisH · 2021-06-23T16:03:32Z

This does fix #1032 for me. I do not understand the change, though.

KyleDavisSA · 2021-06-24T06:08:10Z

For the quasi-Newton scheme, the x^0 variable is always zero unless the initial write data is specified. In PR #1004, the initial write data is read in, but is not stored in the variable cplData->previousIteration for the QN method. That means in the concatenateCouplingData function in BaseQNAcceleration, x^0 is always zero and never updated with the initial write data.

precice/src/acceleration/BaseQNAcceleration.cpp

Line 466 in bd0e158

const auto &oldValues = cplData[id]->previousIteration();

The 1D elastic tube test case is sensitive to this initial write data. The function storeIteration() updates the previousIteration values in:

precice/src/cplscheme/CouplingData.cpp

Lines 38 to 41 in bd0e158

    
           void CouplingData::storeIteration() 
        
           { 
        
             _previousIteration = this->values(); 
        
           }

Adding this during exchangeInitialData solved this problem.

BenjaminRodenberg · 2021-06-24T08:00:52Z

Good find and thanks for the explanation! This is something I oversaw in #1004.

Any idea why some of the tests are failing now? Do you think we should write a specific test to avoid this kind of bug in the future? I'll look into it more closely tomorrow.

KyleDavisSA · 2021-06-24T09:40:27Z

I think a test is necessary. I will look into a test, that writes initial data and checks that value against x^0 values.

BenjaminRodenberg · 2021-06-24T10:40:42Z

I think a test is necessary. I will look into a test, that writes initial data and checks that value against x^0 values.

Then I will wait until you trigger me again or something pops up here. Right?

BenjaminRodenberg

I guess we also have to add storeIteration at the right places in MultiCouplingScheme:

precice/src/cplscheme/MultiCouplingScheme.cpp

Lines 67 to 82 in 54707b3

    
           void MultiCouplingScheme::exchangeInitialData() 
        
           { 
        
             PRECICE_ASSERT(isImplicitCouplingScheme(), "MultiCouplingScheme is always Implicit."); 
        
             if (receivesInitializedData()) { 
        
               for (size_t i = 0; i < _m2ns.size(); i++) { 
        
                 receiveData(_m2ns[i], _receiveDataVector[i]); 
        
               } 
        
               checkDataHasBeenReceived(); 
        
             } 
        
             if (sendsInitializedData()) { 
        
               for (size_t i = 0; i < _m2ns.size(); i++) { 
        
                 sendData(_m2ns[i], _sendDataVector[i]); 
        
               } 
        
             } 
        
           }

KyleDavisSA · 2021-06-30T08:20:24Z

Is there a potential bug in the SerialTests? Below is an output from the test:

/__w/precice/precice/src/precice/tests/SerialTests.cpp(401): error: in "PreciceTests/Serial/testExplicitWithDataInitialization": check 2.0 == valueDataB has failed [2 != 0]. Absolute value exceeds tolerance [|2| > 1e-09]

SolverB writes the initial data of 2

precice/src/precice/tests/SerialTests.cpp

Lines 418 to 421 in f71e555

    
           cplInterface.writeScalarData(dataBID, 0, 2.0); 
        
           //sagen dass daten jetzt geschrieben 
        
           cplInterface.markActionFulfilled(precice::constants::actionWriteInitialData()); 
        
           cplInterface.initializeData();

and SolverA reads in the initial data

precice/src/precice/tests/SerialTests.cpp

Lines 398 to 401 in f71e555

    
           double valueDataB = 0.0; 
        
           cplInterface.initializeData(); 
        
           cplInterface.readScalarData(dataBID, 0, valueDataB); 
        
           BOOST_TEST(2.0 == valueDataB);

However the test still says the the solverB data is zero. SolverA should not be reading the values before SolverB sets them?

uekerman · 2021-07-02T16:16:47Z

However the test still says the the solverB data is zero. SolverA should not be reading the values before SolverB sets them?

Not sure I understand what you mean, but the test looks correct. Not the test is buggy, preCICE is buggy.

fsimonis · 2021-07-26T07:36:42Z

Shouldn't the receiving serial solver skip the call to initializeData?

Sender / Second:

initialize
is initializeData required?
write Data(2)
initializeData
mark as done

Receiver / First:

initialize (receives here)
read data = 2

Actually, initializeData should throw an error if it is called without being required to.

BenjaminRodenberg · 2021-07-28T15:03:51Z

I did another quick test comparing 3ff0899 (pre #1004) and 54707b3 (current state of this PR): I ran the 1d-elastic-tube cpp version precice/tutorials@46ce8ab and compared the precice-{Fluid,Solid}-iterations.log. Here nothing suspicous shows up. I interpret this as a good sign and will dig deeper.

…to storeWriteInitialData

BenjaminRodenberg · 2021-07-29T09:22:13Z

With this version the tests work and also the elastic-tube is ok. However, I would like to add a test that catches the underlying problem. I'm also not yet sure whether actually all additions to the code are necessary.

Todos

Develop a test that fails for 40eca9d and succeeds for 8197993. See Extend test for initializeData for coupling schemes #1050.
Refactor Adds storeIteration for initial write data #1036, if necessary

BenjaminRodenberg · 2021-07-29T19:41:37Z

This PR is from my perspective ready. I would still suggest to merge #1050 first.

KyleDavisSA · 2021-09-13T06:45:12Z

This PR closes #1034 and #1032, but does it also work in the expected manner for the newly constructed tests in PR #1050 ?

All tests pass now with this PR.

BenjaminRodenberg · 2021-09-13T07:48:10Z

This PR closes #1034 and #1032, but does it also work in the expected manner for the newly constructed tests in PR #1050 ?

All tests pass now with this PR.

When I tried this out a while ago: yes. You can locally merge this branch into i_1032_test (#1050) an run the tests. Then all tests (especially the previously failing ones) should succeed.

fsimonis · 2021-09-13T15:29:43Z

Can we merge the tests into this PR, see if all is well and finally merge the fix?

Adds storeIteration for initial write data

54707b3

KyleDavisSA requested review from BenjaminRodenberg and uekerman June 23, 2021 18:06

BenjaminRodenberg reviewed Jun 24, 2021

View reviewed changes

BenjaminRodenberg mentioned this pull request Jun 25, 2021

Incorrect data / IQN error in elastic-tube-1d in develop #1032

Closed

fsimonis added this to the Version 2.3.0 milestone Jun 30, 2021

Merge branch 'develop' into storeWriteInitialData

c9c90b2

BenjaminRodenberg mentioned this pull request Jul 29, 2021

elastic-tube-1d: Unnecessary readBlockScalarData precice/tutorials#231

Closed

BenjaminRodenberg added 2 commits July 29, 2021 11:15

Fix cplscheme.

67c3a9e

Merge branch 'storeWriteInitialData' of github.com:precice/precice in…

8197993

…to storeWriteInitialData

BenjaminRodenberg added 4 commits July 29, 2021 13:50

Give test access to CouplingData.

a4ff272

Check for correct values in previousIteration.

61b50ed

Add a test for ParallelCouplingScheme.

d8ec1e8

Formatting and check sizes.

7b0e164

BenjaminRodenberg mentioned this pull request Jul 29, 2021

Extend test for initializeData for coupling schemes #1050

Merged

7 tasks

BenjaminRodenberg added 3 commits July 29, 2021 21:21

Initialize _previousIteration with zeros.

e3aafdc

Remove todos.

b0a4b18

Refactor storeIteration into BaseCouplingScheme.

7687122

Simplify test.

d8d6af4

BenjaminRodenberg self-assigned this Jul 29, 2021

BenjaminRodenberg added 5 commits August 13, 2021 13:26

Use fixture instead of friend in tests.

906dd95

Merge branch 'develop' into i_1032_test

db615b9

Capitalize struct.

d6342d3

Move fixtures into testing.

deae723

Cleanup and formatting.

063b553

uekerman removed their request for review September 13, 2021 08:42

Merge branch 'i_1032_test' into storeWriteInitialData

af612b9

fsimonis merged commit 8f67136 into develop Sep 13, 2021

BenjaminRodenberg deleted the storeWriteInitialData branch November 18, 2021 14:30

	void MultiCouplingScheme::exchangeInitialData()
	{
	PRECICE_ASSERT(isImplicitCouplingScheme(), "MultiCouplingScheme is always Implicit.");

	if (receivesInitializedData()) {
	for (size_t i = 0; i < _m2ns.size(); i++) {
	receiveData(_m2ns[i], _receiveDataVector[i]);
	}
	checkDataHasBeenReceived();
	}
	if (sendsInitializedData()) {
	for (size_t i = 0; i < _m2ns.size(); i++) {
	sendData(_m2ns[i], _sendDataVector[i]);
	}
	}
	}

Uh oh!

Conversation

KyleDavisSA commented Jun 23, 2021 • edited by BenjaminRodenberg Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Main changes of this PR

Motivation and additional information

Author's checklist

Reviewers' checklist

Uh oh!

MakisH commented Jun 23, 2021

Uh oh!

KyleDavisSA commented Jun 24, 2021

Uh oh!

BenjaminRodenberg commented Jun 24, 2021

Uh oh!

KyleDavisSA commented Jun 24, 2021

Uh oh!

BenjaminRodenberg commented Jun 24, 2021

Uh oh!

BenjaminRodenberg left a comment

Choose a reason for hiding this comment

Uh oh!

KyleDavisSA commented Jun 30, 2021

Uh oh!

uekerman commented Jul 2, 2021

Uh oh!

fsimonis commented Jul 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BenjaminRodenberg commented Jul 28, 2021

Uh oh!

BenjaminRodenberg commented Jul 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Todos

Uh oh!

BenjaminRodenberg commented Jul 29, 2021

Uh oh!

KyleDavisSA commented Sep 13, 2021

Uh oh!

BenjaminRodenberg commented Sep 13, 2021

Uh oh!

fsimonis commented Sep 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

KyleDavisSA commented Jun 23, 2021 •

edited by BenjaminRodenberg

Loading

fsimonis commented Jul 26, 2021 •

edited

Loading

BenjaminRodenberg commented Jul 29, 2021 •

edited

Loading