Skip to content

Migrate API from IDs to names#1588

Merged
fsimonis merged 34 commits intoprecice:developfrom
fsimonis:migrate-ids-to-names
Mar 8, 2023
Merged

Migrate API from IDs to names#1588
fsimonis merged 34 commits intoprecice:developfrom
fsimonis:migrate-ids-to-names

Conversation

@fsimonis
Copy link
Copy Markdown
Member

@fsimonis fsimonis commented Mar 2, 2023 •
edited by davidscn
Loading

Main changes of this PR

This PR migrates the API from using IDs for meshes and data to using names instead.
This entailed:

  • updating the Participant from identifying mesh and data ids to using names
  • updating the entire API incluing bindings
  • updating the API validation methods to checking for names. They now emit useful hints in addition to only stating the problem.

Motivation and additional information

Incorrect meshnames now display hints for the following cases:

  • Only one known mesh
  • Typoed name of a mesh with an edit distance less than 3
  • No extended help: lists all known mesh names

Incorrect data names now display hints for the following cases:

  • Only one known data for the given mesh
  • Exact data name on another mesh
  • Typoed name of a data from this mesh with an edit distance less than 3
  • No extended help: lists all known data names of the given mesh

Resolves #1471
Resolves #1461

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 fsimonis requested a review from uekerman March 2, 2023 10:45
@fsimonis fsimonis added breaking change Breaks backwards compatibility and users need to act enhancement A new feature, a new functionality of preCICE (from user perspective) labels Mar 2, 2023
@fsimonis fsimonis added this to the Version 3.0.0 milestone Mar 2, 2023
uekerman
uekerman requested changes Mar 3, 2023
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 already. I did, however, not have a look at every detail.

It could be dangerous to shorten meshName to mesh in the API. Users could understand they need to pass the positions here. In the Fortran bindings, meshLength sounds a lot like the number of vertices in a mesh.

Same for data vs dataName, data could be misinterpreted as the actual values.

Many tests still use "meshID" or "dataID" as variable names.

Comment thread src/precice/SolverInterface.hpp Outdated
Comment thread src/precice/tests/ParallelTests.cpp Outdated
Comment thread tests/parallel/GlobalRBFPartitioning.cpp Outdated
Comment thread src/cplscheme/config/CouplingSchemeConfiguration.hpp Outdated
Comment thread src/precice/impl/Participant.hpp
Comment thread src/precice/impl/Participant.hpp
Comment thread extras/bindings/fortran/src/SolverInterfaceFortran.cpp Outdated
@fsimonis
Copy link
Copy Markdown
Member Author

fsimonis commented Mar 3, 2023 •
edited
Loading

The argument name is actually a very good point. It also leads to many name clashes in our API functions, where we get the actual data from the read/write contexts.

@fsimonis fsimonis requested a review from uekerman March 4, 2023 15:16
@fsimonis
Copy link
Copy Markdown
Member Author

fsimonis commented Mar 4, 2023

@uekerman I migrated the names in all test now.

@davidscn davidscn marked this pull request as ready for review March 8, 2023 11:45
@davidscn davidscn requested a review from IshaanDesai March 8, 2023 11:47
IshaanDesai
IshaanDesai reviewed Mar 8, 2023
View reviewed changes
Comment thread extras/bindings/c/include/precice/SolverInterfaceC.h Outdated
IshaanDesai
IshaanDesai reviewed Mar 8, 2023
View reviewed changes
Comment thread extras/bindings/c/include/precice/SolverInterfaceC.h Outdated
IshaanDesai
IshaanDesai reviewed Mar 8, 2023
View reviewed changes
Comment thread extras/bindings/c/include/precice/SolverInterfaceC.h Outdated
IshaanDesai
IshaanDesai reviewed Mar 8, 2023
View reviewed changes
Comment thread extras/bindings/c/include/precice/SolverInterfaceC.h Outdated
IshaanDesai
IshaanDesai reviewed Mar 8, 2023
View reviewed changes
Comment thread extras/bindings/c/include/precice/SolverInterfaceC.h
IshaanDesai
IshaanDesai reviewed Mar 8, 2023
View reviewed changes
Comment thread extras/bindings/fortran/include/precice/SolverInterfaceFortran.hpp
IshaanDesai
IshaanDesai reviewed Mar 8, 2023
View reviewed changes
Comment thread extras/bindings/fortran/include/precice/SolverInterfaceFortran.hpp
IshaanDesai
IshaanDesai reviewed Mar 8, 2023
View reviewed changes
Comment thread extras/bindings/fortran/include/precice/SolverInterfaceFortran.hpp
IshaanDesai
IshaanDesai reviewed Mar 8, 2023
View reviewed changes
Comment thread extras/bindings/fortran/include/precice/SolverInterfaceFortran.hpp
IshaanDesai
IshaanDesai reviewed Mar 8, 2023
View reviewed changes
Comment thread extras/bindings/fortran/include/precice/SolverInterfaceFortran.hpp
IshaanDesai
IshaanDesai reviewed Mar 8, 2023
View reviewed changes
Comment thread extras/bindings/fortran/include/precice/SolverInterfaceFortran.hpp
IshaanDesai
IshaanDesai reviewed Mar 8, 2023
View reviewed changes
Comment thread src/cplscheme/config/CouplingSchemeConfiguration.cpp
IshaanDesai
IshaanDesai reviewed Mar 8, 2023
View reviewed changes
Comment thread src/mesh/Mesh.cpp
IshaanDesai
IshaanDesai reviewed Mar 8, 2023
View reviewed changes
Comment thread src/precice/SolverInterface.hpp Outdated
IshaanDesai
IshaanDesai reviewed Mar 8, 2023
View reviewed changes
Comment thread src/precice/SolverInterface.hpp
IshaanDesai
IshaanDesai reviewed Mar 8, 2023
View reviewed changes
Comment thread src/precice/impl/Participant.cpp
IshaanDesai
IshaanDesai reviewed Mar 8, 2023
View reviewed changes
Comment thread src/precice/impl/Participant.cpp
IshaanDesai
IshaanDesai reviewed Mar 8, 2023
View reviewed changes
Comment thread src/precice/impl/Participant.cpp
IshaanDesai
IshaanDesai reviewed Mar 8, 2023
View reviewed changes
Comment thread src/precice/impl/SolverInterfaceImpl.cpp Outdated
IshaanDesai
IshaanDesai reviewed Mar 8, 2023
View reviewed changes
Comment thread tests/parallel/GlobalRBFPartitioning.cpp Outdated
IshaanDesai
IshaanDesai reviewed Mar 8, 2023
View reviewed changes
Comment thread tests/parallel/GlobalRBFPartitioning.cpp Outdated
IshaanDesai
IshaanDesai approved these changes Mar 8, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@IshaanDesai IshaanDesai left a comment

Choose a reason for hiding this comment

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

Looks solid 👍 some minor suggestions and concerns from my side.

@fsimonis fsimonis merged commit e095779 into precice:develop Mar 8, 2023
@IshaanDesai IshaanDesai mentioned this pull request Mar 9, 2023
Merged
erikscheurer added a commit to precice/PreCICE.jl that referenced this pull request Mar 13, 2023
@erikscheurer
[WIP] Use names instead of IDs related to precice/precice#1588
c78feb3
IshaanDesai pushed a commit to precice/PreCICE.jl that referenced this pull request Mar 16, 2023
@erikscheurer
First part of making develop compatible with precice:develop (#47)
95ee249 
* Remove mapWriteDataFrom (precice/precice#1222) and Vertex-only API (precice/precice#1322)

* Remove `initializeData` (precice/precice#1350)

* Remove `isReadDataAvailable` and `isWriteDataRequired` (precice/precice#1362)

* Rename `isGradientDataRequired` (precice/precice@632b4fb)

* Remove `isActionRequired` and replace with explicit calls

* Adjust Solverdummies and tests

* Format changes

* [WIP]  Use names instead of IDs  related to precice/precice#1588

* Delete functions not part of the precice API

* Implement API changes into tests

* Format
davidscn
davidscn reviewed Mar 20, 2023
View reviewed changes
Comment thread docs/changelog/1587.md
@@ -0,0 +1 @@
- Removed deprecated `getDataIds()`
Copy link
Copy Markdown
Member

@davidscn davidscn Mar 20, 2023

Choose a reason for hiding this comment

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

This changelog entry is actually important and not complete. The PR here also removes getMeshID (getDataID)

@uekerman uekerman mentioned this pull request Mar 31, 2023
Closed
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@davidscn davidscn davidscn left review comments

@IshaanDesai IshaanDesai IshaanDesai approved these changes

@uekerman uekerman Awaiting requested review from uekerman

+1 more reviewer

@BenjaminRodenberg BenjaminRodenberg BenjaminRodenberg left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@fsimonis fsimonis

Labels

breaking change Breaks backwards compatibility and users need to act enhancement A new feature, a new functionality of preCICE (from user perspective)

Projects

None yet

Milestone

Version 3.0.0

Development

Successfully merging this pull request may close these issues.

Port API from const string to string_view Use names instead of IDs in API

5 participants

@fsimonis @uekerman @BenjaminRodenberg @davidscn @IshaanDesai