Remove Multi- and BiCouplingScheme by BenjaminRodenberg · Pull Request #1432 · precice/precice

BenjaminRodenberg · 2022-09-10T09:44:12Z

Main changes of this PR

Merge Multi- and ParallelCouplingScheme
Remove BiCouplingScheme in favor of SerialCouplingScheme
Simplifies class hierarchy and limits amount of duplicate code

Motivation and additional information

Removes a lot of code and reduces the implementation effort necessary for #1414. On the long run I assume that the MultiCouplingScheme will also profit from this change, because it gets automatically tested by the ParallelCouplingScheme.

Author's checklist

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.
I ran make format to ensure everything is formatted correctly.
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?

* Remove mapReadDataTo and mapWriteDataFrom. * Remove tests using removed API.

* Merge initialize and initializeData in SolverInterfaceImpl. * Merge initialize and initializeData in CouplingScheme. * Update tests correspondingly * Allow to check actions before initialize, especially checking whether initial data has to be written * Check for sending and receiving of initial data during coupling scheme configuration * Code simplification w.r.t waveform. * Use dt=0.0 in actions in initialize. Co-authored-by: Benjamin Uekermann <[email protected]> Co-authored-by: Frédéric Simonis <[email protected]>

…recice#1352) * Get tests and configs for serial implicit waveform interpolation from precice#1029. * Differentiate between received initial data and data (from advance). * Split data communication for SerialCouplingScheme and store initial data in waveform. * Move common functionality of serial and parallel coupling scheme into BiCouplingScheme. Co-authored-by: Benjamin Uekermann <[email protected]>

* Add test Integration/Serial/InitializeData/ImplicitBoth where both participants initialize. * Allow initialize=true for both participants in serial-implicit coupling scheme. * Raise warning, if first participant in serial coupling scheme initializes data with waveform order zero. * Remove check for write data, because participant does not know about waveform order. Co-authored-by: Benjamin Uekermann <[email protected]>

precice#1349)

Co-authored-by: Benjamin Uekermann <[email protected]>

* Make read mapping conditional to avoid unnecessary mappings. * Update corresponding tests. Co-authored-by: Benjamin Uekermann <[email protected]>

* Merge Multi- and ParallelCouplingScheme * Remove BiCouplingScheme in favor of SerialCouplingScheme * Simplifies class hierarchy and limits amount of duplicate code

* Remove mapReadDataTo and mapWriteDataFrom. * Remove tests using removed API.

* Merge initialize and initializeData in SolverInterfaceImpl. * Merge initialize and initializeData in CouplingScheme. * Update tests correspondingly * Allow to check actions before initialize, especially checking whether initial data has to be written * Check for sending and receiving of initial data during coupling scheme configuration * Code simplification w.r.t waveform. * Use dt=0.0 in actions in initialize. Co-authored-by: Benjamin Uekermann <[email protected]> Co-authored-by: Frédéric Simonis <[email protected]>

…recice#1352) * Get tests and configs for serial implicit waveform interpolation from precice#1029. * Differentiate between received initial data and data (from advance). * Split data communication for SerialCouplingScheme and store initial data in waveform. * Move common functionality of serial and parallel coupling scheme into BiCouplingScheme. Co-authored-by: Benjamin Uekermann <[email protected]>

* Add test Integration/Serial/InitializeData/ImplicitBoth where both participants initialize. * Allow initialize=true for both participants in serial-implicit coupling scheme. * Raise warning, if first participant in serial coupling scheme initializes data with waveform order zero. * Remove check for write data, because participant does not know about waveform order. Co-authored-by: Benjamin Uekermann <[email protected]>

precice#1349)

Co-authored-by: Benjamin Uekermann <[email protected]>

* Make read mapping conditional to avoid unnecessary mappings. * Update corresponding tests. Co-authored-by: Benjamin Uekermann <[email protected]>

uekerman

I see the motivation behind this step (parallel cpl scheme and multi cpl scheme have some similarities), but I must admit I am not too convinced that this step will help us in the long run 😟

The suggested changes reduce some duplicated code (but not that much), but make the new version harder to understand (why does a serial cpl scheme have a single m2n, but a parallel cpl scheme many?).

I my opinion, the old version was easier to read and understand. Sometimes code duplication is good if it makes things more understandable.

Additional disadvantage: configuration and classes are no longer alligned (there is a cplscheme::multi but no MultiCouplingScheme.

uekerman · 2022-09-19T09:54:22Z

+  /// Local participant name.
+  std::string _localParticipant = "unknown";
+
+  /// name of the controller participant ( = second participant in serial bi coupling scheme)


Suggested change

/// name of the controller participant ( = second participant in serial bi coupling scheme)

/// name of the controller participant ( = second participant in serial or parallel coupling scheme)

uekerman · 2022-09-19T09:56:13Z

+
+  std::vector<PtrCouplingData> allReceiveCouplingData();
+
+  std::vector<PtrCouplingData> allCouplingDataWithId(DataID dataId);


Can there be multiple data with the same data id? I guess the name is a bit confusing. And we have getSendData and getReceiveData already.

uekerman · 2022-09-19T09:58:35Z

-   * @brief Checks whether any CouplingData in dataMap requires initialization
-   * @param dataMap map containing CouplingData
-   * @return true, if any CouplingData in dataMap requires initialization
+   * @brief Checks whether any CouplingData in vector requires initialization


Suggested change

* @brief Checks whether any CouplingData in vector requires initialization

* @brief Checks whether any CouplingData requires initialization

uekerman · 2022-09-19T09:59:15Z

+   * @return true, if any CouplingData in vector requires initialization
   */
-  bool anyDataRequiresInitialization(DataMap &dataMap) const;
+  bool anyDataRequiresInitialization(std::vector<PtrCouplingData> datas) const;


Suggested change

bool anyDataRequiresInitialization(std::vector<PtrCouplingData> datas) const;

bool anyDataRequiresInitialization(std::vector<PtrCouplingData> dataVector) const;

uekerman · 2022-09-19T10:01:44Z

                 "Maximal iteration limit has to be larger than zero.");

+  if (_controller == _localParticipant) {
+    // Controller participant never does the first step, because it is never the first participant


Suggested change

// Controller participant never does the first step, because it is never the first participant

A does A because it is A ... not needed I guess.

uekerman · 2022-09-19T10:09:00Z

+  /// Sets the values
+  CouplingData *getSendData(DataID dataID);


Comment wrong?

uekerman · 2022-09-19T10:15:06Z

+  auto dataWithId = allCouplingDataWithId(dataID);
+  PRECICE_ASSERT(dataWithId.size() >= 1, "Data with given data ID must exist!");
+  for (auto &data : dataWithId) {


Why should there be multiple ones?

uekerman · 2022-09-19T10:17:16Z

+    // @todo implement ParallelCouplingScheme with multi coupling for explicit coupling
+    PRECICE_ASSERT(isImplicitCouplingScheme(), "ParallelCouplingScheme with multi coupling is always Implicit.");
+  }
+  PRECICE_DEBUG("MultiCoupling scheme is created for {}.", localParticipant);


Suggested change

PRECICE_DEBUG("MultiCoupling scheme is created for {}.", localParticipant);

uekerman · 2022-09-19T10:18:37Z


 bool ParallelCouplingScheme::exchangeDataAndAccelerate()
 {
+  PRECICE_DEBUG("Computed full length of iteration");


What do you mean with "full length of iteration"?

uekerman · 2022-09-19T10:31:43Z

      const std::string &accessor) const;

-  PtrCouplingScheme createMultiCouplingScheme(
+  PtrCouplingScheme createParallelMultiCouplingScheme(


Suggested change

PtrCouplingScheme createParallelMultiCouplingScheme(

PtrCouplingScheme createParallelCouplingScheme(

BenjaminRodenberg · 2022-09-20T08:13:30Z

After discussion with @uekerman: Let's keep this PR as it is for the moment. The change is definitely not a clear thing, but useful during the development on #1414 (it reduces code duplication and simplifies changes in BenjaminRodenberg#13). As soon as BenjaminRodenberg#13 has converged, we can either include the refactoring of #1432 or drop it (and implement the relevant changes from BenjaminRodenberg#13 in #1414).

uekerman · 2022-11-21T10:17:49Z

Closing for now as we decided against this refactoring.

BenjaminRodenberg and others added 9 commits August 22, 2022 21:47

Remove mapWriteDataFrom and mapReadDataTo (precice#1222)

5beb7b3

* Remove mapReadDataTo and mapWriteDataFrom. * Remove tests using removed API.

Remove isReadDataAvailable and isWriteDataRequired (precice#1362)

9fb5d8a

Add integration test to reproduce first-participant + initData problem (

567b265

precice#1349)

Simplify signature of actions and remove ScaleByDtAction (precice#1403)

9f0b1d1

Co-authored-by: Benjamin Uekermann <[email protected]>

Make read mapping conditional. (precice#1404)

f3ef21d

* Make read mapping conditional to avoid unnecessary mappings. * Update corresponding tests. Co-authored-by: Benjamin Uekermann <[email protected]>

Remove Multi- and BiCouplingScheme

fc0dae9

* Merge Multi- and ParallelCouplingScheme * Remove BiCouplingScheme in favor of SerialCouplingScheme * Simplifies class hierarchy and limits amount of duplicate code

BenjaminRodenberg added the maintainability Working on this will make our lives easier in the long run as preCICE gets easier to maintain. label Sep 10, 2022

BenjaminRodenberg requested a review from uekerman September 10, 2022 09:44

BenjaminRodenberg self-assigned this Sep 10, 2022

BenjaminRodenberg mentioned this pull request Sep 10, 2022

**OUTDATED** writeData when subcycling is used #1414

Closed

17 tasks

BenjaminRodenberg and others added 8 commits September 15, 2022 20:30

Remove mapWriteDataFrom and mapReadDataTo (precice#1222)

dd50bd5

* Remove mapReadDataTo and mapWriteDataFrom. * Remove tests using removed API.

Remove isReadDataAvailable and isWriteDataRequired (precice#1362)

8ef6386

Add integration test to reproduce first-participant + initData problem (

3bab6df

precice#1349)

Simplify signature of actions and remove ScaleByDtAction (precice#1403)

a14e553

Co-authored-by: Benjamin Uekermann <[email protected]>

Make read mapping conditional. (precice#1404)

85c325a

* Make read mapping conditional to avoid unnecessary mappings. * Update corresponding tests. Co-authored-by: Benjamin Uekermann <[email protected]>

BenjaminRodenberg force-pushed the develop-v3.0.0 branch from f3ef21d to 85c325a Compare September 15, 2022 18:46

BenjaminRodenberg added 2 commits September 15, 2022 20:58

Merge branch 'develop-v3.0.0' into remove-bi-and-multi-coupling-scheme

a375cc7

Fix error.

7b285fe

BenjaminRodenberg added this to the Version 3.0.0 milestone Sep 17, 2022

uekerman reviewed Sep 19, 2022

View reviewed changes

BenjaminRodenberg force-pushed the develop-v3.0.0 branch 2 times, most recently from 721976f to c5c7b50 Compare November 4, 2022 16:43

fsimonis marked this pull request as draft November 17, 2022 12:17

uekerman closed this Nov 21, 2022

BenjaminRodenberg mentioned this pull request Dec 2, 2022

Remove BiCouplingScheme, merge MultiCouplingScheme and ParallelCouplingScheme BenjaminRodenberg/precice#13

Closed

8 tasks

BenjaminRodenberg mentioned this pull request Dec 22, 2022

WIP: write data when subcycling is used (outdated) #1523

Closed

25 tasks

BenjaminRodenberg mentioned this pull request Feb 9, 2023

Refactor controller in multi coupling-scheme config #1560

Closed

BenjaminRodenberg mentioned this pull request Nov 22, 2024

Fix initial data in multi-coupling schemes #2144

Merged

7 tasks

	/// name of the controller participant ( = second participant in serial bi coupling scheme)
	/// name of the controller participant ( = second participant in serial or parallel coupling scheme)


		std::vector<PtrCouplingData> allReceiveCouplingData();

		std::vector<PtrCouplingData> allCouplingDataWithId(DataID dataId);

	* @brief Checks whether any CouplingData in vector requires initialization
	* @brief Checks whether any CouplingData requires initialization

	bool anyDataRequiresInitialization(std::vector<PtrCouplingData> datas) const;
	bool anyDataRequiresInitialization(std::vector<PtrCouplingData> dataVector) const;

		/// Sets the values
		CouplingData *getSendData(DataID dataID);

	PtrCouplingScheme createParallelMultiCouplingScheme(
	PtrCouplingScheme createParallelCouplingScheme(

Uh oh!

Conversation

BenjaminRodenberg commented Sep 10, 2022

Main changes of this PR

Motivation and additional information

Author's checklist

Reviewers' checklist

Uh oh!

uekerman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BenjaminRodenberg commented Sep 20, 2022

Uh oh!

uekerman commented Nov 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants