elastic-tube-1d: Unnecessary readBlockScalarData

[BenjaminRodenberg]

The following readBlockScalarData statements looks wrong and is inconsistent with the python version of the 1d tube:

tutorials/elastic-tube-1d/fluid-cpp/src/FluidSolver.cpp

Lines 104 to 106 in 6d90ae3

	if (interface.isReadDataAvailable()) {
	interface.readBlockScalarData(crossSectionLengthID, chunkLength, vertexIDs.data(), crossSectionLength.data());
	}

Compared to the original code (see https://github.com/precice/elastictube1d/blob/master/fluid-cpp/FluidSolver.cpp) this statement has been added. Is there a specific reason?

I did some experiments related to this change and will document them here, since this might be helpful for precice/precice#1036:

commit	comment	fluid-cpp + solid-cpp remove statement above (fa75a39)	fluid-cpp + solid-cpp keep statement above (46ce8ab)	fluid-python + solid-python (46ce8ab)
precice/precice@3ff0899	pre precice/precice#1004	✔️	✔️	✔️
precice/precice@54707b3	current state precice/precice#1036	✔️	✔️	✔️
precice/precice@40eca9d	current state precice:develop	❌	❌	❌
precice/precice@c9c90b2	merge precice:develop into precice/precice#1036	✔️	✔️	✔️

✔️ -> elastic-tube-1d works as expected
❌ -> elastic-tube-1d breaks as described in precice/precice#1032

[MakisH]

I got very confused on what you meant by "looks wrong". I guess you want to point out that it is duplicate, as it appears again later inside the same loop.

I am not sure what the experiments are showing (what does ❌ mean?), but it anyway indeed looks duplicate and thus wrong.

[BenjaminRodenberg]

I am not sure what the experiments are showing (what does x mean?), but it anyway indeed looks duplicate and thus wrong.

I added a statement below the table.

[fsimonis]

Why does this read statement look wrong?

This looks like textbook API usage

• initialize
• initializeData
• read if available (ok, this is not required)
• while coupling ongoing
• create checkpoint if required
• read if available
• solve
• write if required
• advance
• restore checkpoint if required

The read data call that looks weird to me is

tutorials/elastic-tube-1d/fluid-cpp/src/FluidSolver.cpp

Lines 127 to 130 in 6d90ae3

	//interface.readBlockScalarData(crossSectionLengthID, chunkLength, vertexIDs.data(), crossSectionLength.data());
	if (interface.isReadDataAvailable()) {
	interface.readBlockScalarData(crossSectionLengthID, chunkLength, vertexIDs.data(), crossSectionLength.data());
	}

[uekerman]

There are definitely too many reads in there, but it is not forbidden to call read multiple times. This should never influence the results.

initialize
while(ongoing){
  read
  solve
  write
  advance
}

and

initialize
read
while(ongoing){
  solve
  write
  advance
  read
}

are both correct and allowed.

[BenjaminRodenberg]

Quick summary:

• There are too many reads (maybe "incorrect" was a bit too strong in the header of this Issue)
• There are two possibilities (elastic-tube-1d: Unnecessary readBlockScalarData #231 (comment)), current implementation is a mixture of both.

What do you think about the solution I implemented in #232? Feel free to leave a review. I change the title of this issue for more clearity.

[fsimonis]

So, this means that we have a bug in precice. The elastictube1d is just a bit overly verbose.

So we should implement tests that read and write multiple times.

[BenjaminRodenberg]

So, this means that we have a bug in precice. The elastictube1d is just a bit overly verbose.

Yes. But this bug is already documented in precice/precice#1032, a fix is on the way (precice/precice#1036) and a test, as well (precice/precice#1050). Or do you mean a different bug?

So we should implement tests that read and write multiple times.

Opened an issue on this: precice/precice#1055

[MakisH]

Closed by #232