Indirect access #2099

davidscn · 2024-10-11T12:40:24Z

Main changes of this PR

Implements a read-consistent and write-conservative indirect access for nearest neighbor and PU-RBF.

@uekerman I know the naming in the mapping classes is not consistent at the moment. The conceptual idea in the DataContext and the interplay with waveforms should be final, however. As discussed there, we do a sample, then map.

Nearest-neighbor mapping (read and write)
PU-RBF mapping (read and write)
Testing: serial nearest-neighbor mapping (read and write)
Testing: serial PU-RBF mapping (read and write)
Parallization/repartitioning
Testing: parallel nearest-neighbor mapping (read)
Testing: parallel PU-RBF mapping (read and write)
More extensive parallel testing

Motivation and additional information

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 stuck to C++17 features.
I stuck to CMake version 3.22.1.
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?

uekerman

I finally managed to have a look. Sorry for the delay.
Good job already 👍

I did not yet look into the PUM and RBF implementation details and into the tests.

Currently supported mappings are NN and PUM-RBF. There is currently no safeguard that checks this, right?
How much effort would it be to add NP and global-direct RBF? Not saying that we should add them now, but to better understand.

I would suggest to make the feature experimental for the time being and add safeguards to the API functions.

Could you please start working on a docs page in parallel? That would also simplify reviewing.

uekerman · 2024-11-26T08:09:32Z

src/mapping/Mapping.hpp

+  /// Returns true if either the input or output is an indirect (dummy) mesh
+  bool isIndirectMapping() const;


Clear what an "indirect mesh" is? Could be helpful to add more docs here.

uekerman · 2024-11-26T08:09:55Z

src/mapping/Mapping.hpp

+  virtual void writeConservativeAt(::precice::span<const double> coordinates, Eigen::Map<const Eigen::MatrixXd> &source, Eigen::Map<Eigen::MatrixXd> &target);
+
+  // @todo consider making this a private method in the RBFMapping/PUM mapping class
+  virtual void updateMappingDataCache(MappingDataCache &cache, Eigen::VectorXd &in);
+
+  // For now only for read-consistent
+  virtual void evaluateMappingDataCacheAt(::precice::span<const double> coordinates, const MappingDataCache &cache, ::precice::span<double> values);


Please add (significant) docs to these functions and then copy-doc to all mapping implementations.

uekerman · 2024-11-26T08:19:24Z

src/mapping/NearestNeighborBaseMapping.cpp

+  // @TODO: How to handle the parallel partitioning for indirect access here?
+  // in the usual case, we make use of the indexSet, which is pre-computed from the mapping
+  // for the indirect access, we can't do that since we don't have the output mesh
+  // what we would need to do in theory:
+  // find all nearest-neighbors at the 'boundary' of the access region, which would require an
+  // infinite fine sampling of output mesh nodes wo be used in the computeMapping below
+  // for now, we simply tag everything and move on.
+  if (this->isIndirectMapping()) {
+    // Depending on the mapping constraint, one of these tagAll calls will do nothing, as the vertex
+    // set of the mesh is empty. From a practical point of view, we only need to apply the
+    // tagging to one of the meshes (the remote one). But calling it on both sides reliefs us from any
+    // conditional code.
+    output()->tagAll();
+    input()->tagAll();
+    return;
+  }


IMO, it is the right approach here to tag everything.

find all nearest-neighbors at the 'boundary' of the access region

This depends on how we interpret the access region. Is this the region where a user can call access any location or is this the region where the other mesh is accessed. In the latter case, the user needs to be aware that operating close to the boundary of the box can lead to a bad mapping. Meaning: use a safety margin.
I had this second interpretation in mind.

uekerman · 2024-11-26T08:39:24Z

src/mapping/PartitionOfUnityMapping.hpp

+template <typename RADIAL_BASIS_FUNCTION_T>
+void PartitionOfUnityMapping<RADIAL_BASIS_FUNCTION_T>::updateMappingDataCache(MappingDataCache &cache, Eigen::VectorXd &in)
+{
+  // We cannot synchronize this event, as the call might to this function is rank-local only


Suggested change

// We cannot synchronize this event, as the call might to this function is rank-local only

// We cannot synchronize this event, as the call to this function is rank-local only

uekerman · 2024-11-26T08:42:39Z

src/mapping/PartitionOfUnityMapping.hpp

+
+  // Step 1: use the indexed center mesh to find relevant cluster
+  query::Index &clusterIndex = _centerMesh->index();
+  // Step 4: find all clusters the output vertex lies in, i.e., find all cluster centers which have the distance of a cluster radius from the given output vertex


uekerman · 2024-11-26T09:16:21Z

src/precice/config/ParticipantConfiguration.cpp

                    "Participant \"{}\" has a read mapping to mesh \"{}\", without providing it. "
                    "Please add a provide-mesh tag with name=\"{}\"",
                    participant->getName(), toMesh, toMesh);
    } else {
      // A write mapping maps from provided to received
-      PRECICE_CHECK(participant->isMeshProvided(fromMesh),
+      // The indirect cannot be on the "to" mesh, as only the combinations read-consistent and write conservative are allowed


Suggested change

// The indirect cannot be on the "to" mesh, as only the combinations read-consistent and write conservative are allowed

// The indirect cannot be on the "to" mesh, as only the combinations read-consistent and write-conservative are allowed

uekerman · 2024-11-26T09:17:42Z

src/precice/config/ParticipantConfiguration.cpp

+    // The mesh context associated to the mappings
+    // impl::MeshContext &fromMeshContext = ;
+    // impl::MeshContext &toMeshContext   = ;



uekerman · 2024-11-26T09:25:05Z

src/precice/impl/DataContext.hpp

+   * For conventional mappings, multiple MappingContexts can only occur in write direction (write
+   * to one local mesh, map it to different remote meshes). Since the indirect mapping operates


I am not sure if this statement is correct.
https://github.com/precice/precice/blob/develop/tests/serial/multiple-mappings/MultipleReadToMappings.xml

uekerman · 2024-11-26T09:26:17Z

src/precice/impl/ParticipantImpl.cpp

+                dataDims, dataName, meshName,
+                coordinates.size() / dim, values.size(), expectedDataSize, dataDims, coordinates.size() / dim);
+
+  // TODO: Add check that this vertex is within the access region?


uekerman · 2024-11-26T09:29:10Z

src/mapping/MappingDataCache.hpp

+ * The usual mapping data structures have no notion about the underlying data.
+ * The Cache stores in addition a timestep the data corresponds to.
+ */
+struct MappingDataCache {


This class is currently specific to PUM, right?
This "only" works since NN doesn't need one and you have not yet touched the others, right?

In any case, would it makes sense to move this into impl?

davidscn added the enhancement A new feature, a new functionality of preCICE (from user perspective) label Oct 11, 2024

davidscn self-assigned this Oct 11, 2024

davidscn requested a review from uekerman October 11, 2024 12:42

davidscn force-pushed the indirect-access branch from ae7ccbe to 6008ee1 Compare October 28, 2024 09:46

davidscn mentioned this pull request Oct 30, 2024

More refined repartitioning for direct access #2121

Merged

davidscn force-pushed the indirect-access branch from 7cd3e67 to d4f41c7 Compare October 30, 2024 08:21

uekerman requested changes Nov 26, 2024

View reviewed changes

davidscn added 23 commits December 17, 2024 12:17

Provide some basic test

e9bfdf2

Add API for jit reading

d40332b

Small fixup of loop

378d829

Provide a dummy implementation using NN

9923084

Rename API function

65c3d35

WIP

7578b37

Small working example

5057e1c

Add basic API methods for writing data

90dbac1

Add write NN support with test

cbb3774

Make explicit read test case stronger

2ca9cb4

Support for 2D and 3D meshes

adeffe3

Make testcase more strong

6cdb050

Extend test significantly

e8c0b7e

Use comparison to conventional methods

8740b63

Add proper testing for implicit coupling

c5368c0

Remove obsolete TODO

9e5463b

[tag] nearest-neighbor version

9a5ec4c

Virtualize indirect mapping methods

15b5272

Add PUM Test

16aaf84

Implement PUM for scalar read consistent

0b517c1

Extend PUM to vectorial test case

80ff0b9

Bring it to work with vector data

8d65f63

Extend the PUM direct access test

b25bdde

davidscn added 12 commits December 17, 2024 12:41

Fix check for duplicate to and from meshes with indirect access mappings

654b82b

Add comment on alternative data layout

0c399ab

Add some events to measure the performance

ddcd74e

Add a consistency check for shape of input data

f3ee7e4

Add event for naive cache update

b647e88

Indentation and formatting changes

c2c1b78

Provide a failing distributed test

b4c8b9f

Add naive tagging for NN + indirect access

4b1e227

Wreorder

daee17b

Apply safety factor scaling for indirect access

04feaee

Test two timesteps

87afbdb

Update test signature

8b0c481

davidscn force-pushed the indirect-access branch from 89efcba to 8b0c481 Compare December 17, 2024 11:48

davidscn added 2 commits December 17, 2024 12:59

Typo ranks vs rank

fbdd1a5

Fix participant name in parallel test

931ff90

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Indirect access #2099

Indirect access #2099

davidscn commented Oct 11, 2024 •

edited by uekerman

Loading

uekerman left a comment

uekerman Nov 26, 2024

uekerman Nov 26, 2024

uekerman Nov 26, 2024

uekerman Nov 26, 2024

uekerman Nov 26, 2024

uekerman Nov 26, 2024

uekerman Nov 26, 2024

uekerman Nov 26, 2024

uekerman Nov 26, 2024

uekerman Nov 26, 2024

		/// Returns true if either the input or output is an indirect (dummy) mesh
		bool isIndirectMapping() const;

	// We cannot synchronize this event, as the call might to this function is rank-local only
	// We cannot synchronize this event, as the call to this function is rank-local only

	// The indirect cannot be on the "to" mesh, as only the combinations read-consistent and write conservative are allowed
	// The indirect cannot be on the "to" mesh, as only the combinations read-consistent and write-conservative are allowed

		* For conventional mappings, multiple MappingContexts can only occur in write direction (write
		* to one local mesh, map it to different remote meshes). Since the indirect mapping operates

Indirect access #2099

Are you sure you want to change the base?

Indirect access #2099

Conversation

davidscn commented Oct 11, 2024 • edited by uekerman Loading

Main changes of this PR

Motivation and additional information

Author's checklist

Reviewers' checklist

uekerman 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

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

davidscn commented Oct 11, 2024 •

edited by uekerman

Loading