Skip to content

Simplify implementation of data mapping in SolverInterfaceImpl#1216

Merged
BenjaminRodenberg merged 17 commits intodevelopfrom
refactor-solver-interface-impl-map-data
Mar 26, 2022
Merged

Simplify implementation of data mapping in SolverInterfaceImpl#1216
BenjaminRodenberg merged 17 commits intodevelopfrom
refactor-solver-interface-impl-map-data

Conversation

@BenjaminRodenberg
Copy link
Copy Markdown
Contributor

@BenjaminRodenberg BenjaminRodenberg commented Mar 23, 2022 •
edited
Loading

Main changes of this PR

Creates function isMappingRequired and uses mapData everywhere, where mapping is performed.

Motivation and additional information

Branched off from #1187

Author's checklist

  • Move isMappingRequired into DataContext?
  • Move mapData into DataContext?
  • Move most of the implementation of SolverInterfaceImpl::resetWrittenData into DataContext?
  • I added a changelog file with make changelog if there are user-observable changes since the last release.
  • 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?

@BenjaminRodenberg BenjaminRodenberg added the maintainability Working on this will make our lives easier in the long run as preCICE gets easier to maintain. label Mar 23, 2022
@BenjaminRodenberg BenjaminRodenberg self-assigned this Mar 23, 2022
@BenjaminRodenberg BenjaminRodenberg mentioned this pull request Mar 23, 2022
Merged
12 tasks
@BenjaminRodenberg
Copy link
Copy Markdown
Contributor Author

BenjaminRodenberg commented Mar 23, 2022

@uekerman I realized that we could also move mapData into DataContext. I think we already had this discussion previously, but it would allow us to delete some more functions (mappingContext()) of DataContext or make them private and avoid indirections (context.mappingContext().mapping->map(fromDataID, toDataID)).

I'm also fine with the state as it is, but I currently do not really see why moving isMappingRequired is fine and moving mapData is not.

fsimonis
fsimonis reviewed Mar 23, 2022
View reviewed changes
@fsimonis fsimonis requested review from davidscn and removed request for uekerman March 23, 2022 10:02
davidscn
davidscn reviewed Mar 23, 2022
View reviewed changes
src/precice/impl/SolverInterfaceImpl.cpp
context.resetToData();
PRECICE_DEBUG("Map from dataID {} to dataID: {}", inDataID, outDataID);
context.mappingContext().mapping->map(inDataID, outDataID);
PRECICE_DEBUG("Mapped values = {}", utils::previewRange(3, context.toData()->values())); // @todo might be better to move this debug message into Mapping::map and remove getter DataContext::toData()
Copy link
Copy Markdown
Member

@davidscn davidscn Mar 23, 2022

Choose a reason for hiding this comment

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

Why do you remove this DEBUG print? I think it might be useful. I think it might be useful.

Copy link
Copy Markdown
Contributor Author

@BenjaminRodenberg BenjaminRodenberg Mar 23, 2022

Choose a reason for hiding this comment

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

Good that you saw this. I removed it because it's the only place where DataContext::toData() is actually used. Maintaining this public function just for a debug statement is in my opinion too much and therefore I removed the function and the debug statement. Also fixed this now in 39910cc and cleaned it up.

If somebody really needs to debug here, this can also happen somewhere else where this data is actually used and accessed, for example inside the mapping.

Copy link
Copy Markdown
Member

@uekerman uekerman Mar 25, 2022

Choose a reason for hiding this comment

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

IIRC we added these Debug statements here in a dedicated effort by @fsimonis
These Debug statements are really important when developing an adapter. Let's please not remove them.

Copy link
Copy Markdown
Contributor Author

@BenjaminRodenberg BenjaminRodenberg Mar 25, 2022

Choose a reason for hiding this comment

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

Moved the debug statement inside DataContext then we also don't need DataContext::toData().

@BenjaminRodenberg BenjaminRodenberg changed the title Simplify implementation of data mapping Simplify implementation of data mapping in SolverInterfaceImpl Mar 23, 2022
@BenjaminRodenberg BenjaminRodenberg mentioned this pull request Mar 23, 2022
Closed
8 tasks
@fsimonis fsimonis removed their request for review March 24, 2022 15:48
@fsimonis
Copy link
Copy Markdown
Member

fsimonis commented Mar 24, 2022

I leave the review to @davidscn. LGTM

uekerman
uekerman requested changes Mar 25, 2022
View reviewed 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.

Looks good expect the removed mapping preview.

@uekerman
Copy link
Copy Markdown
Member

uekerman commented Mar 25, 2022

@uekerman I realized that we could also move mapData into DataContext. I think we already had this discussion previously, but it would allow us to delete some more functions (mappingContext()) of DataContext or make them private and avoid indirections (context.mappingContext().mapping->map(fromDataID, toDataID)).

Both are OK. In a previous version you hid the mapping completely in the DataContext class, meaning that there was nothing like "map data" in SolverInterfaceImpl. This, I didn't like because it should be easy to discover where the mapping happens when skimming SolverInterfaceImpl. This is no longer the case in #1217, so no concerns from my side.

@BenjaminRodenberg
Copy link
Copy Markdown
Contributor Author

BenjaminRodenberg commented Mar 25, 2022

@davidscn can you take another look at this PR?

davidscn
davidscn reviewed Mar 25, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@davidscn davidscn left a comment

Choose a reason for hiding this comment

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

Only a minor comment, otherwise feel free to merge.

@BenjaminRodenberg BenjaminRodenberg merged commit 354851e into develop Mar 26, 2022
@BenjaminRodenberg BenjaminRodenberg deleted the refactor-solver-interface-impl-map-data branch March 26, 2022 14:14
@BenjaminRodenberg BenjaminRodenberg mentioned this pull request Mar 26, 2022
Closed
7 tasks
@fsimonis fsimonis added this to the Version 2.4.0 milestone May 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fsimonis fsimonis fsimonis left review comments

@davidscn davidscn davidscn left review comments

@uekerman uekerman uekerman approved these changes

Assignees

@BenjaminRodenberg BenjaminRodenberg

Labels

maintainability Working on this will make our lives easier in the long run as preCICE gets easier to maintain.

Projects

None yet

Milestone

Version 2.4.0

Development

Successfully merging this pull request may close these issues.

4 participants

@BenjaminRodenberg @fsimonis @uekerman @davidscn