Skip to content

Add tests for non-experimental API and subcycling.#1186

Merged
BenjaminRodenberg merged 10 commits intoprecice:developfrom
BenjaminRodenberg:tests-for-non-experimental-subcycling
Mar 22, 2022
Merged

Add tests for non-experimental API and subcycling.#1186
BenjaminRodenberg merged 10 commits intoprecice:developfrom
BenjaminRodenberg:tests-for-non-experimental-subcycling

Conversation

@BenjaminRodenberg
Copy link
Copy Markdown
Contributor

Main changes of this PR

Rearrange existing tests for subcycling and add new ones for different coupling schemes.

Motivation and additional information

Adding these tests helps to ensure that #1029 does not introduce a regression or accidentally adds new features, if experimental=false.

Author's checklist

  • I added a changelog file with make changelog if there are user-observable changes since the last release. (no user-side changes)
  • I ran make format to ensure everything is formatted correctly.
  • I sticked to C++14 features.
  • I sticked to CMake version 3.10.
  • 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?

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 only had a detailed look at one of the tests. Not outsourcing common parts of tests into generic functions, I guess is OK here. But this should not mean that we copy unnecessary code parts, see below. Goal should be that each individual test is as easy to understand as possible. Once it fails, we want to understand what the test does 😁

@BenjaminRodenberg
Copy link
Copy Markdown
Contributor Author

@uekerman Do you think there are other tests that we would need here to make sure we don't break the classical subcycling? The only thing I can imagine is data initialization with subcycling. But this should already be covered by other tests.

We could also shift some tests from other places over here. I just looked into SerialTests.cpp and ParallelTests.cpp for subcycling and found the following test.

/**
* @brief Buggy simulation setup of FSI coupling between Flite and Calculix.
*
* Bug: after first call of advance by Flite the mapped forces are value NaN.
*
* Some information about the coupling:
* - explicit coupling scheme
* - Flite (incompressible Navier-Stokes) starts simulation
* - Mapping is done on Flite side with RBF
*
* @todo rename this test and config
*/
BOOST_AUTO_TEST_CASE(testBug)

If the main purpose of this test is not subcycling, then we should leave it as it is (in this PR).

@BenjaminRodenberg BenjaminRodenberg added this to the Version 2.4.0 milestone Mar 9, 2022
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.

Much easier to understand now 👍

@uekerman Do you think there are other tests that we would need here to make sure we don't break the classical subcycling? The only thing I can imagine is data initialization with subcycling. But this should already be covered by other tests.

Maybe testing a CompositionalCouplingScheme case? Not much more comes to mind.

Flite and Calculix testcase

That's still one from Bernhard. Not sure whether this one is about subcycling mainly. I guess you have a better overview.

What's the difference between DoNothingWithSubcycling and ReadWriteScalarDataWIthSubcycling? Why does the one only exist for serial coupling? Do we need both? If yes, maybe we could find better names.

@BenjaminRodenberg
Copy link
Copy Markdown
Contributor Author

What's the difference between DoNothingWithSubcycling and ReadWriteScalarDataWIthSubcycling? Why does the one only exist for serial coupling? Do we need both? If yes, maybe we could find better names.

Removed this test in 381c7de. I was hesitant about removing an existing test, but if you also don't really see a purpose for this test anymore, we can remove it. I think the test ReadWriteScalarDataWIthSubcycling tests the functionality of DoNothingWithSubcycling and even more.

@BenjaminRodenberg
Copy link
Copy Markdown
Contributor Author

Flite and Calculix testcase

That's still one from Bernhard. Not sure whether this one is about subcycling mainly. I guess you have a better overview.

Opened an issue. I think we should delete it, because I don't really see why we need it. #1214

@BenjaminRodenberg
Copy link
Copy Markdown
Contributor Author

Maybe testing a CompositionalCouplingScheme case? Not much more comes to mind.

Added a very simple for-sake-of-completeness test in 29c6ae2. Doing something more exciting would require rewriting most of the test and we are testing compositional coupling schemes somewhere else. I think to make sure that subcycling does not do something strange this should be enough (e.g. counting iterations twice or not at all, if a compositional scheme is used).

@BenjaminRodenberg BenjaminRodenberg merged commit df2aa99 into precice:develop Mar 22, 2022
@BenjaminRodenberg BenjaminRodenberg deleted the tests-for-non-experimental-subcycling branch March 22, 2022 13:33
BenjaminRodenberg added a commit to BenjaminRodenberg/precice that referenced this pull request Mar 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants