Add global/meshless data #1549

kanishkbh · 2023-01-19T16:44:15Z

Adds functionality to exchange data that is not associated with any mesh. So far the name is "global-data".

Motivation and additional information

Closes #202

Main changes of this PR

API
- Provide methods to read/write global data.
- Initialize and advance global data.
Configuration
- Allow reading the xml tag global-data
- Allow participants to configure read/write global data without specifying any mesh name.
- Allow coupling to configure exchanges for global data without specifying any mesh name.
- ~~Allow implicit coupling to configure convergence measures for global data without specifying any mesh name.~~ (Open TODO for a future PR)
Data
- ~~Add a GlobalData class (meshless equivalent of Data).~~
- DataConfiguration:
  - Configure the global-data tag.
  - Add a vector _globalData in DataConfiguration to store global data here as Data objects.
Participants
- Add a GlobalRead/WriteDataContexts class (meshless and mappingless equivalents of Read/WriteDataContext) which inherit from DataContext.
- ParticipantState and ParticipantConfiguration: Configure and store global read/write contexts.
Coupling
- ~~Add a GlobalCouplingData class, analogous to CouplingDatafor mesh data.~~ Add an isGlobal attribute to CouplingData and use it for global coupling as well.
- CouplingSchemeConfiguration: Add a struct to store global exchanges and functions for global data (analogous to mesh-associated data).
Com
- M2N: always use intracom in case of global data.

Open Questions and TODOs:

Documented in #1716

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?

src/mesh/GlobalData.cpp

fsimonis · 2023-03-10T19:12:27Z

The many changes are due to the github pr pointing to develop-3.0.0 instead of develop.
Just letting you know 😉

uekerman

Had a first more thorough look at this. Nice that it works for simple problems already 👍

Please rebase often. A few recent changes might have a big influence here. Good news: I think they simplify things.

The amount of code duplication looks a bit unfortunate. Maybe we could reduce this by defining a common interface for both data types (mesh-based and global)?
@fsimonis Any quick suggestions?

Connected to the discussion in #202, "size" is currently fixed at 1. In my opinion, this is completely OK as a first step. Let's only try to generalize this later if required.

Probably clear, but we need to increase test coverage, especially tests that combine global and mesh-based data. Also, the combination with waveform iteration and data initialization needs testing.

In general, please don't commit commented-out code. Only leads to confusion in the long run.

src/precice/SolverInterface.hpp

tests/global-data/Explicit.cpp

src/precice/impl/SolverInterfaceImpl.cpp

src/cplscheme/config/CouplingSchemeConfiguration.hpp

uekerman · 2023-04-03T16:34:12Z

There is actually no real difference between Data and GlobalData. There is even no pointer to Mesh. We could simply reuse Data and set _hasGradient to false (a valid case already).

kanishkbh · 2023-04-04T02:40:06Z

There is actually no real difference between Data and GlobalData. There is even no pointer to Mesh. We could simply reuse Data and set _hasGradient to false (a valid case already).

Yes agree; I also felt and suggested the same back in January. Will check now if changing all GlobalData instances back to Data is quick and straightforward.

And actually this is only one of the multiple classes/structs associated with global data that are just slightly modified equivalents of their mesh-data cousins. So there is more potential for removing code duplication, but others might not be so straightfoward as reusing Data.

BenjaminRodenberg

I added a few comments on the question whether there should be a GlobalDataContext or rather a GlobalReadDataContext and GlobalWriteDataContext.

src/precice/impl/GlobalDataContext.hpp

BenjaminRodenberg · 2023-04-04T09:18:39Z

src/precice/impl/GlobalDataContext.hpp

+ * - GlobalDataContext is basically a stripped-down equivalent of DataContext.
+ *   Unlike DataContext, it has no associated mappings or meshes.
+ */
+class GlobalDataContext {


Why does GlobalDataContext not inherit from DataContext? (compared to ReadDataContext and WriteDataContext)

DataContext has a pointer _mesh and mesh-associated query functions which were not needed for global, so I created a new GlobalDataContext (just like GlobalData was created instead of reusing Data).

But now following the same philosophy of getting rid of GlobalData and making Data itself versatile, we can do the same here, i.e., reuse DataContext for global by for e.g. setting _mesh to nullptr in case of global contexts.

But now following the same philosophy of getting rid of GlobalData and making Data itself versatile, we can do the same here, i.e., reuse DataContext for global by for e.g. setting _mesh to nullptr in case of global contexts.

Sounds to me like a risky approach: We then get a general Data that is either implicitly global (has, for example, empty gradients and no _mesh) or lives on a mesh. To me this sounds like there should be an AbstractData which provides the base for MeshData and GlobalData. But not sure about all the implications this might have, though.

For now I've made Data work for both mesh and global datas, and it looks good.
Regarding an AabstractData: The current Data class has no knowledge of any mesh (Datas don't point to any meshes. Only meshes point to their Datas).
Since Data is already dumb, it makes sense to me to use it as it is for both mesh-associated and global use cases.

src/precice/impl/GlobalDataContext.hpp

src/precice/impl/Participant.hpp

kanishkbh · 2023-05-10T17:53:16Z

src/precice/impl/DataContext.hpp

-   * @param[in] mappingContext Context of the mapping
-   * @param[in] meshContext Context of mesh this mapping is mapping from or to
-   */
-  virtual void appendMappingConfiguration(MappingContext &mappingContext, const MeshContext &meshContext) = 0;


Explanation: This function is removed because Global read/write DataContexts inherit from DataContext, but don't implement appendMappingConfiguration(...)

But then you have to declare this function in ReadDataContext and WriteDataContext. We could, of course, introduce an intermediate layer LocalDataContext just for this purpose, but this looks overengineered. You can also just implement it in Read/WriteGlobalDataContext and trigger an assertion, if somebody tries to call the function.

In the end this is a hint for some problem with the class design, but I think we can live with it.

uekerman

I am really sorry, but I think we need another rebase. But those massive changes should soon be over 🙈

In general, please resolve done conversations. Such that we can keep a better overview of what we still need to do.

To reduce some of the code duplication here (e.g. convergence measures), maybe we could restrict the feature to explicit coupling for the moment??

Before merging we definitely need:

Integration tests that combine mesh-based and global data
Mark feature as experimental

Open TODOS that could also come later, but should be documented in a follow-up issue:

src/precice/SolverInterface.hpp

src/precice/impl/DataContext.cpp

kanishkbh · 2023-06-10T14:06:45Z

To reduce some of the code duplication here (e.g. convergence measures), maybe we could restrict the feature to explicit coupling for the moment??

We can also allow global data exchange during implicit coupling without offering convergence measures for global data.
Implemented this in the last commit. In the configuration if a convergence measure is defined for a global data, it will throw an error.

src/precice/impl/ParticipantImpl.cpp

src/precice/Participant.hpp

src/mesh/config/DataConfiguration.cpp

src/m2n/M2N.cpp

src/cplscheme/config/CouplingSchemeConfiguration.hpp

src/cplscheme/config/CouplingSchemeConfiguration.cpp

uekerman

Couldn't we save a lot of code duplication if we create an abstract bases class CouplingData from which then the current CouplingData (needs a different name then) and GlobalCouplingData inherits? Then, we don't need a GlobalDataMap etc

src/cplscheme/BaseCouplingScheme.hpp

src/cplscheme/BaseCouplingScheme.cpp

src/cplscheme/SerialCouplingScheme.cpp

src/cplscheme/config/CouplingSchemeConfiguration.cpp

Following precice#1549 (comment)

Ref precice#1549 (comment)

kanishkbh · 2023-06-22T14:04:27Z

@uekerman

Couldn't we save a lot of code duplication if we create an abstract bases class CouplingData from which then the current CouplingData (needs a different name then) and GlobalCouplingData inherits? Then, we don't need a GlobalDataMap etc

Ah yes. There was actually nothing new in GlobalCouplingData, so I've removed it altogether.
Now CouplingData class is being used for both mesh and global data (like DataContext).

Drawback: No more PRECICE_ASSERT(_mesh) in CouplingData's constructor (just like DataContext).

uekerman · 2023-06-23T10:24:57Z

Ah yes. There was actually nothing new in GlobalCouplingData, so I've removed it altogether.
Now CouplingData class is being used for both mesh and global data (like DataContext).

Sounds good 👍

Drawback: No more PRECICE_ASSERT(_mesh) in CouplingData's constructor (just like DataContext).

-> sth for the follow-up issue

src/cplscheme/BaseCouplingScheme.cpp

Following precice#1549 (comment)

Ref precice#1549 (comment)

…d rename `waveform-order` to `waveform-degree`

tests/serial/global-data/ExplicitInitializeData.cpp

uekerman

In a good-enough state to merge from my perspective. Remaining TODOs are documented in #1716.

Thanks for the patient and extensive contribution @kanishkbh 🚀

@BenjaminRodenberg Please check again the interplay with waveforms. Are we missing anything?

@fsimonis Please have a brief general look again. Anything we might regret in the future?

tests/serial/global-data/ExplicitWithDirectMeshAccess.cpp

src/cplscheme/config/CouplingSchemeConfiguration.cpp

src/cplscheme/MultiCouplingScheme.cpp

src/cplscheme/BaseCouplingScheme.cpp

BenjaminRodenberg

I think some tests using a MultiCouplingScheme or CompositionalCouplingScheme would be good, because this is often a place where the integration of experimental features causes problems. Besides that: I don't see a problem right now. The amount of code-duplication will require some refactoring in the future, but that's ok.

BenjaminRodenberg · 2023-07-12T07:54:37Z

src/cplscheme/BaseCouplingScheme.cpp

+    if (not data->isGlobal()) {
+      meshID = data->getMeshID();
+    } else {
+      meshID = -1;


Using something like mesh::GLOBAL_DATA_ID would be better here. (compare to time::Storage::WINDOW_START)

Agree. Added a mesh::Mesh::GLOBAL_DATA_MESH_ID{-2}
(-1 is reserved for MESH_ID_UNDEFINED)

BenjaminRodenberg · 2023-07-12T07:57:23Z

src/cplscheme/BaseCouplingScheme.cpp

+  // TODO: store mesh and global data together in a `DataMap _allData ` (currently produces a bug), see https://github.com/precice/precice/issues/1716
+  if (isGlobal) {
+    if (!utils::contained(id, _allGlobalData)) { // data is not used by this coupling scheme yet, create new CouplingData
+      _allGlobalData.emplace(id, ptrCplData);
+    } else { // data is already used by another exchange of this coupling scheme, use existing CouplingData
+      ptrCplData = _allGlobalData[id];
+    }
+  } else {                                     // mesh data
+    if (!utils::contained(id, _allMeshData)) { // data is not used by this coupling scheme yet, create new CouplingData
+      _allMeshData.emplace(id, ptrCplData);
+    } else { // data is already used by another exchange of this coupling scheme, use existing CouplingData
+      ptrCplData = _allMeshData[id];
+    }
  }


Any chance to try to merge _allGlobalData and _allMeshData like suggested above? I guess otherwise we will just keep it the way it is for a very long time.

BenjaminRodenberg · 2023-07-12T07:59:32Z

src/cplscheme/BaseCouplingScheme.cpp

-  for (auto &data : _allData | boost::adaptors::map_values) {
+  for (auto &data : _allMeshData | boost::adaptors::map_values) {
+    data->moveToNextWindow();
+  }
+  for (auto &data : _allGlobalData | boost::adaptors::map_values) {
    data->moveToNextWindow();
  }


If you cannot put both into _allData, maybe try to use a function allData() that returns _allMeshData + _allGlobalData? (maybe this also leads to the same bug again)

BenjaminRodenberg · 2023-07-12T08:00:19Z

src/cplscheme/BaseCouplingScheme.hpp

-  /// All send and receive data as a map "data ID -> data"
-  DataMap _allData;
+  /// All send and receive mesh-associated data as a map "data ID -> data"
+  DataMap _allMeshData;
+  /// All send and receive global data as a map "data ID -> data"
+  DataMap _allGlobalData;


I would suggest to add the comment TODO ... here instead.

BenjaminRodenberg · 2023-07-12T08:14:45Z

src/precice/impl/DataContext.hpp

-   * @param[in] mappingContext Context of the mapping
-   * @param[in] meshContext Context of mesh this mapping is mapping from or to
-   */
-  virtual void appendMappingConfiguration(MappingContext &mappingContext, const MeshContext &meshContext) = 0;


But then you have to declare this function in ReadDataContext and WriteDataContext. We could, of course, introduce an intermediate layer LocalDataContext just for this purpose, but this looks overengineered. You can also just implement it in Read/WriteGlobalDataContext and trigger an assertion, if somebody tries to call the function.

In the end this is a hint for some problem with the class design, but I think we can live with it.

fsimonis

I would prefer to refactor this PR before we merge it as I see the risk of introducing a lot of preventable duplication, which could easily diverge.

We definitely still need:

Tests for Multi Coupling schemes
Tests for Compositional schemes

Refactoring on my list:

Simplify the implementation of the configuration
Merge data and global data containers
Simplify DataContexts
Investigate if we can make Data self-aware if it is global or not.

fsimonis · 2023-07-19T07:56:57Z

src/cplscheme/BaseCouplingScheme.cpp

@@ -227,16 +240,26 @@ void BaseCouplingScheme::initializeWithZeroInitialData(const DataMap &receiveDat
  }
 }

-PtrCouplingData BaseCouplingScheme::addCouplingData(const mesh::PtrData &data, mesh::PtrMesh mesh, bool requiresInitialization, bool communicateSubsteps)
+PtrCouplingData BaseCouplingScheme::addCouplingData(const mesh::PtrData &data, mesh::PtrMesh mesh, bool requiresInitialization, bool communicateSubsteps, bool isGlobal)


This would become way simpler if Data would know if it is global or attached to a mesh.

uekerman · 2023-07-21T12:09:42Z

Tests for Multi Coupling schemes

Multi coupling is not yet supported in this PR, see #1716

fsimonis · 2023-08-21T14:15:19Z

In #1742, we moved the dimensions attribute to the mesh, meaning that we don't have a global dimensionality to work with.

So <global-data:vector /> is ambiguous now.

At this point, we can either make the vectorial dimension explicit,

<global-data:scalar />
<global-data:vector2d />
<global-data:vector3d />

or we make the entire dimensionality explicit, allowing global data of any dimensions pretty naturally and requiring only a single tag.

<global-data dimensions="1" />
<global-data dimensions="2" />
<global-data dimensions="3" />

uekerman · 2023-08-21T15:16:54Z

Good point.

Alternatively, if we want to keep things simple here and extend later, we could restrict the feature to scalar first.

<global-data:scalar />

In principle,

<global-data dimensions="n" />

sounds good, but might lead to (much?) additional work if we allow any n.

MakisH · 2023-08-22T16:05:49Z

At this point, we can either make the vectorial dimension explicit,
<global-data:scalar />
<global-data:vector2d />
<global-data:vector3d />

Sounds inconsistent with the <mesh/> tag.

or we make the entire dimensionality explicit, allowing global data of any dimensions pretty naturally and requiring only a single tag.
<global-data dimensions="1" />
<global-data dimensions="2" />
<global-data dimensions="3" />

Sounds more natural.

Alternatively, if we want to keep things simple here and extend later, we could restrict the feature to scalar first.
<global-data:scalar />

I like the "let's try things slowly" approach.

In principle,
<global-data dimensions="n" />
sounds good, but might lead to (much?) additional work if we allow any n.

Why the too much additional work (compared to vector2d, vector3d)? I would assume that this is the simpler solution.

uekerman · 2023-08-24T07:03:42Z

Why the too much additional work (compared to vector2d, vector3d)? I would assume that this is the simpler solution.

Just had a look again: It should actually be easier than I originally expected. Might only need changes in the DataConfiguration class.

github-actions bot assigned kanishkbh Jan 19, 2023

uekerman reviewed Jan 20, 2023

View reviewed changes

src/mesh/GlobalData.cpp Outdated Show resolved Hide resolved

uekerman mentioned this pull request Feb 15, 2023

Write results of forces function object to preCICE precice/openfoam-adapter#277

Open

kanishkbh force-pushed the add-global-data branch from 0603299 to cf02bf3 Compare March 7, 2023 14:57

kanishkbh changed the base branch from develop-v3.0.0 to develop March 16, 2023 15:34

uekerman requested changes Mar 31, 2023

View reviewed changes

BenjaminRodenberg reviewed Apr 4, 2023

View reviewed changes

kanishkbh force-pushed the add-global-data branch from 33552c0 to 0576ed3 Compare April 20, 2023 14:38

kanishkbh force-pushed the add-global-data branch from e45654f to 5f26ca4 Compare May 9, 2023 13:20

kanishkbh commented May 10, 2023

View reviewed changes

uekerman reviewed May 19, 2023

View reviewed changes

src/precice/SolverInterface.hpp Outdated Show resolved Hide resolved

src/precice/impl/DataContext.cpp Show resolved Hide resolved

kanishkbh force-pushed the add-global-data branch from 55ca9fa to 3fac740 Compare May 20, 2023 18:39

kanishkbh force-pushed the add-global-data branch from 37dd7a7 to 3453ef0 Compare June 1, 2023 20:35

uekerman reviewed Jun 16, 2023

View reviewed changes

uekerman reviewed Jun 19, 2023

View reviewed changes

kanishkbh added a commit to kanishkbh/precice that referenced this pull request Jun 21, 2023

Merge a send-data query function for mesh and global data.

ec22703

Following precice#1549 (comment)

kanishkbh added a commit to kanishkbh/precice that referenced this pull request Jun 21, 2023

Add missing moveToNextWindow() call for global data.

bed0343

Following precice#1549 (comment)

kanishkbh added a commit to kanishkbh/precice that referenced this pull request Jun 21, 2023

Small correction in documentation.

b3d17c2

Ref precice#1549 (comment)

kanishkbh mentioned this pull request Jun 22, 2023

Add global data to the documentation precice/precice.github.io#279

Merged

uekerman reviewed Jun 23, 2023

View reviewed changes

src/cplscheme/BaseCouplingScheme.cpp Outdated Show resolved Hide resolved

kanishkbh added a commit to kanishkbh/precice that referenced this pull request Jul 6, 2023

Merge a send-data query function for mesh and global data.

b711248

Following precice#1549 (comment)

kanishkbh added a commit to kanishkbh/precice that referenced this pull request Jul 6, 2023

Add missing moveToNextWindow() call for global data.

1726bdc

Following precice#1549 (comment)

kanishkbh added a commit to kanishkbh/precice that referenced this pull request Jul 6, 2023

Small correction in documentation.

4ac7253

Ref precice#1549 (comment)

kanishkbh force-pushed the add-global-data branch from 072668c to 04d3600 Compare July 6, 2023 18:30

Port to: (precice#1714) Make Waveform accessible from CouplingData an…

c179e93

…d rename `waveform-order` to `waveform-degree`

kanishkbh force-pushed the add-global-data branch from 0240f05 to c179e93 Compare July 12, 2023 00:22

kanishkbh added 2 commits July 12, 2023 02:51

Remove unused const getter funcs for Read/WriteGlobalDataContexts

069ad3a

Formatting

50d9c85

kanishkbh marked this pull request as ready for review July 12, 2023 01:00

uekerman reviewed Jul 12, 2023

View reviewed changes

tests/serial/global-data/ExplicitInitializeData.cpp Outdated Show resolved Hide resolved

Update tests/serial/global-data/ExplicitInitializeData.cpp

b50a0c6

uekerman approved these changes Jul 12, 2023

View reviewed changes

uekerman added 5 commits July 12, 2023 09:42

Update tests/serial/global-data/ExplicitWithDirectMeshAccess.cpp

7becc29

Update src/cplscheme/config/CouplingSchemeConfiguration.cpp

f25b23b

Update src/cplscheme/MultiCouplingScheme.cpp

3196e6a

Update src/cplscheme/MultiCouplingScheme.cpp

55124be

Update src/cplscheme/BaseCouplingScheme.cpp

cf9c246

uekerman requested review from BenjaminRodenberg and fsimonis July 12, 2023 07:45

BenjaminRodenberg reviewed Jul 12, 2023

View reviewed changes

Replace meshID -1 with a static constexpr

b742e2e

BenjaminRodenberg mentioned this pull request Jul 12, 2023

Fix initialization for substeps="false" #1719

Merged

9 tasks

fsimonis added this to the Version 3.0.0 milestone Jul 19, 2023

fsimonis added the enhancement A new feature, a new functionality of preCICE (from user perspective) label Jul 19, 2023

fsimonis reviewed Jul 19, 2023

View reviewed changes

uekerman assigned fsimonis Jul 21, 2023

fsimonis modified the milestones: Version 3.0.0, Version 3.x.x Oct 18, 2023

fsimonis mentioned this pull request Apr 24, 2024

Handling global coupling data (not associated to a mesh) #202

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add global/meshless data #1549

Add global/meshless data #1549

kanishkbh commented Jan 19, 2023 •

edited

fsimonis commented Mar 10, 2023

uekerman left a comment

uekerman commented Apr 3, 2023

kanishkbh commented Apr 4, 2023

BenjaminRodenberg left a comment

BenjaminRodenberg Apr 4, 2023

kanishkbh Apr 17, 2023

BenjaminRodenberg Apr 19, 2023

kanishkbh May 5, 2023

kanishkbh May 10, 2023 •

edited

BenjaminRodenberg Jul 12, 2023

uekerman left a comment •

edited

kanishkbh commented Jun 10, 2023

uekerman left a comment

kanishkbh commented Jun 22, 2023

uekerman commented Jun 23, 2023

uekerman left a comment

BenjaminRodenberg left a comment

BenjaminRodenberg Jul 12, 2023

kanishkbh Jul 12, 2023

BenjaminRodenberg Jul 12, 2023

BenjaminRodenberg Jul 12, 2023

BenjaminRodenberg Jul 12, 2023

BenjaminRodenberg Jul 12, 2023

fsimonis left a comment

fsimonis Jul 19, 2023

uekerman commented Jul 21, 2023

fsimonis commented Aug 21, 2023 •

edited

uekerman commented Aug 21, 2023

MakisH commented Aug 22, 2023

uekerman commented Aug 24, 2023

Add global/meshless data #1549

Are you sure you want to change the base?

Add global/meshless data #1549

Conversation

kanishkbh commented Jan 19, 2023 • edited

Motivation and additional information

Main changes of this PR

Open Questions and TODOs:

Author's checklist

Reviewers' checklist

fsimonis commented Mar 10, 2023

uekerman left a comment

Choose a reason for hiding this comment

uekerman commented Apr 3, 2023

kanishkbh commented Apr 4, 2023

BenjaminRodenberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kanishkbh May 10, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uekerman left a comment • edited

Choose a reason for hiding this comment

kanishkbh commented Jun 10, 2023

uekerman left a comment

Choose a reason for hiding this comment

kanishkbh commented Jun 22, 2023

uekerman commented Jun 23, 2023

uekerman left a comment

Choose a reason for hiding this comment

BenjaminRodenberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fsimonis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uekerman commented Jul 21, 2023

fsimonis commented Aug 21, 2023 • edited

uekerman commented Aug 21, 2023

MakisH commented Aug 22, 2023

uekerman commented Aug 24, 2023

kanishkbh commented Jan 19, 2023 •

edited

kanishkbh May 10, 2023 •

edited

uekerman left a comment •

edited

fsimonis commented Aug 21, 2023 •

edited