Skip to content

Samplize mapping#1660

Merged
fsimonis merged 36 commits intodevelopfrom
samplize-mapping
Jun 27, 2023
Merged

Samplize mapping#1660
fsimonis merged 36 commits intodevelopfrom
samplize-mapping

Conversation

@fsimonis
Copy link
Copy Markdown
Member

@fsimonis fsimonis commented May 15, 2023

Main changes of this PR

This PR moves the Mapping to a sample-based interface and adds (real) support for transient mappings.

A sample combines data dimensionality, values and gradients.

General design is now for map():

  • Input is a Sample
  • Output are values

Transient mappings (additional constructor argument) now require to call the second variant of map:

  • Input is a Sample
  • Output are values
  • Input&Output is an initial guess
void Mapping::map(const Sample& inData, Eigen::VectorXd& outData);
void Mapping::map(const Sample& inData, Eigen::VectorXd& outData, Eigen::VectorXd& initialGuess);

Motivation and additional information

DataContext now directly maps samples and creates now output samples.
DataContext also owns the initial guesses for transient mappings.
We reset initial guesses to zero after every time window. So mappings are transient during iterations.

Part of the migration to waveforms.

Closes #1651

PETRBF experiments

Based on v2.5.0, the perpendicular flap tutorial using calclix and nutils:

Tests are:

  • Last iteration as initial guess (v2.5.0)
  • Explicitly set first initial guess to 0
  • No initial guess
  • No initial guess with KSPInitGuessNonZero (no change)
  • Zero initial guess at time window start

Conclusions:

  • We need an initial Guess
  • We use 0 at time window start

petsc-initial-guess-v2

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?

@fsimonis
Copy link
Copy Markdown
Member Author

The PETSc RBF mapping contains two uses of the data IDs to create some kind of cache key.

@uekerman @davidscn Do you have an idea on how to get rid of this?

petsc::Vector &p = std::get<0>( // Save and reuse the solution from the previous iteration
previousSolution.emplace(std::piecewise_construct,
std::forward_as_tuple(inputDataID + outputDataID * 10 + dim * 100),
std::forward_as_tuple(petsc::Vector::allocate(_matrixC, "p"))))

petsc::Vector &out = std::get<0>(
previousSolution.emplace(std::piecewise_construct,
std::forward_as_tuple(inputDataID + outputDataID * 10 + dim * 100),
std::forward_as_tuple(petsc::Vector::allocate(_matrixC, "out"))))
->second;

@uekerman
Copy link
Copy Markdown
Member

uekerman commented May 15, 2023

The PETSc RBF mapping contains two uses of the data IDs to create some kind of cache key.

@uekerman @davidscn Do you have an idea on how to get rid of this?

This is for storing the start value for the iterative solver. Might also be useful for the Gingko CG.
I am not sure how big the influence actually is and whether we ever tested this thoroughly.

@fsimonis
Copy link
Copy Markdown
Member Author

Turns out that we need to add dimensionality information to the Sample.
There are some instances where input partitions can be empty, example being an empty primary partition of the Eigen RadialBasisFunction solver.

@fsimonis
Copy link
Copy Markdown
Member Author

If we want to keep this functionality, we could change the Mapping to:

void Mapping::map(const Sample& input, Eigen::VectorXd& output, std::optional<const Eigen::VectorXd&> lastSolution);
bool Mapping::isIterative() const;

Then in the MappingContext:

if (mapping.isIterative()) {
  mapping.map(inData, outData, _lastSolution);
  _lastSolution = outData;
} else {
  mapping.map(inData, outData);
}

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.

Makes a lot of sense to me.

Work on samples instead of data IDs
Handle initial guesses of transient mappings in the data context
@fsimonis fsimonis force-pushed the samplize-mapping branch from 93c7e18 to 7eee01c Compare May 22, 2023 15:39
@fsimonis
Copy link
Copy Markdown
Member Author

I added the dimensionality to time::Sample and migrated the mapping to it.
DataContext now directly maps samples.

I touched tests only to add the data dimensionality of samples.
All mappings tests need to be migrated in the future. They only need meshes now. No data required.

@fsimonis fsimonis marked this pull request as ready for review May 25, 2023 09:14
@fsimonis fsimonis requested a review from uekerman May 25, 2023 09:14
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.

Looks good, but I only had a quick look.

  • @BenjaminRodenberg FYI changes in Sample etc
  • @davidscn FYI changes to mappings
  • We need a changelog entry as we now treat the initial guesses differently.
  • We need tests for the new solution caching.

@uekerman
Copy link
Copy Markdown
Member

When combining this with waveforms, is there a separate initial guess for each time step? Probably doesn't work as the time grid might be different in each iteration.
Let's document this in a post-v3 issue as it probably needs more investigation.

@fsimonis @BenjaminRodenberg

@fsimonis
Copy link
Copy Markdown
Member Author

When combining this with waveforms, is there a separate initial guess for each time step? Probably doesn't work as the time grid might be different in each iteration.

There is one initial guess per data and mapping. The possibly differing timestamps of the stamples is a reason of why this is impractical to implement.
Currently, we start with an initial guess and update it for every stample in the read mapping.

@fsimonis
Copy link
Copy Markdown
Member Author

Let's document this in a post-v3 issue as it probably needs more investigation.

#1687

Copy link
Copy Markdown
Member

@MakisH MakisH left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I skimmed through all files. The general need is kind of clear: we now have data on different times, so the mapping also needs to work using time samples. Don't rely on my review as an expert view, of course, but only as potentially useful details.

I am not sure I get the motivation and the need for the direct-iterative classification, though (see comment), without being able to suggest any better alternative.

Answering on some questions @fsimonis asked directly to me:

  1. Do the interfaces and assumptions make sense?: To the extent I understand how everything works together, yes. The interface does not look too different than before, it is mostly a matter of working types that change. I am a bit unsure about the time::Sample class (see comment).
  2. Can you see issues that I haven't thought about?: Not really.
  3. How does this behave in the geometric multi-scale situations?: There, we define new mappings, which should all be direct (if I understand the classification). Since there were very few changes in, e.g., the nearest-neighbor mapping, only few changes would be needed in the geometric multiscale as well. There seems to be a notion of dimensions everywhere, so I guess we can work around that. Maybe @ezonta could also comment here.
  4. Is there more information necessary?: I don't see anything right now, but I may need to look at it again once I understand the whole concept better.
  5. Should the return type be a Sample instead of a VectorXD to make the data dimensions explicit?: Why is it currently like this? Wouldn't mapping from a Sample to a Sample (or from a VectorXD to a VectorXD) be the default thought?

@fsimonis fsimonis merged commit 8ea9cd3 into develop Jun 27, 2023
NiklasKotarsky pushed a commit to NiklasKotarsky/precice that referenced this pull request Jul 4, 2023
* Refactor data information out of mapping

Work on samples instead of data IDs
Handle initial guesses of transient mappings in the data context

* Enable initial guess for mu

* Remove mesh integral data versions

* Assert that both input and output contain items

* Port mapping in DataContext to samples

* Add dimension to time::Sample

* Replace mapping::Sample with time::Sample

* Fix include

* Add documentation to Sample

* Extend documentation of Mapping

* Fix log creation for precice without petsc

* Directly map samples

* Remove unused headers

* Remove unpleasant default constructors

* Rename and clarify the initial guess storage

* Fix comment

* Rename transient to iterative in Mapping

* Improve the logging of mappings

* Test different guesses

* Add changelog

* Fix Wording

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

* Reset initial guesses after the read mappings

* Change wording

Co-authored-by: Gerasimos Chourdakis <[email protected]>

* Remove incorrect comment

* Remove dead code comment

* Improve initialGuess comment in PETSc Mapping

* Extend Sample class description

* Remove unused header

* Rename dimension to dimensions in test

* Explain direct mapping

* Reword changelog

* Unify comments

* Rename parameter

* Rename InitialGuessRequirement options

* Renamed Mapping interface regarding isIterative mappings

---------

Co-authored-by: Benjamin Uekermann <[email protected]>
Co-authored-by: Gerasimos Chourdakis <[email protected]>
kanishkbh added a commit to kanishkbh/precice that referenced this pull request Jul 6, 2023
kanishkbh added a commit to kanishkbh/precice that referenced this pull request Jul 12, 2023
@BenjaminRodenberg BenjaminRodenberg deleted the samplize-mapping branch July 27, 2023 08:24
@BenjaminRodenberg BenjaminRodenberg mentioned this pull request Mar 8, 2024
9 tasks
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.

Refactor mapping to work on Storages, not on Data::values

4 participants