Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Try to remove ParticipantImpl::mesh(const std::string &meshName) #1269

Open
BenjaminRodenberg opened this issue Apr 26, 2022 · 2 comments
Open
Labels
maintainability Working on this will make our lives easier in the long run as preCICE gets easier to maintain. question Everybody is invited to answer this question or give any hint.
Milestone

Comments

@BenjaminRodenberg
Copy link
Member

BenjaminRodenberg commented Apr 26, 2022

I stumbled across the public getter ParticipantImpl::mesh(...) when working on the documentation of ParticipantImpl in #1249 and think we should remove this getter, because it is only used in tests and has no proper use-case to justify a public function. This is mostly motivated from a class design perspective trying to simplify ParticipantImpl.

As a compromise, we could also add this function to a fixture or the WhiteboxAccessor, but I think improving the tests would be the better solution. However, for most of the tests I cannot judge whether removing ParticipantImpl::mesh(...) would be ok in the context of the original goal of the test.

I found the following tests that use this function:

tests/serial/whitebox/TestExplicitWithDataScaling.cpp

Easy to remove, because testing::WhiteboxAccessor::impl(cplInterface).mesh("Test-Square-One").vertices().size() always returns nVertices = 4. No need for this function.

tests/parallel/TestFinalize.cpp

What is it actually testing? Can we remove this test?

tests/parallel/NearestProjectionRePartitioning.cpp

I don't understand the rationale here. But is related to #371

/home/benjamin/Programming/precice/tests/serial/mapping-scaled-consistent/helpers.cpp

Do we need to go that low here? For some functions I have the impression the purpose of these helpers lies somewhere else and not on testing the correct creation of the mesh. (Can we just remove the BOOST_REQUIRE calls?)

tests/serial/mapping-nearest-projection/helpers.cpp

At different places. Sometimes a similar situations like above (BOOST_REQUIRE). testQuadMappingNearestProjectionTallKite and testQuadMappingNearestProjectionWideKite are testing mesh::edgeLength(edge). Should this test maybe go somewhere else as a unit test, where accessing the mesh is easier?

@BenjaminRodenberg BenjaminRodenberg added question Everybody is invited to answer this question or give any hint. maintainability Working on this will make our lives easier in the long run as preCICE gets easier to maintain. labels Apr 26, 2022
@BenjaminRodenberg BenjaminRodenberg added this to the Version 2.x.x milestone Apr 26, 2022
@fsimonis
Copy link
Member

fsimonis commented Apr 27, 2022

Looking at the use-cases, they normally ensure that some test conditions are met. This is not strictly necessary and should probably be checked in unit tests instead.
Feel free to remove the checks if you dislike mesh() that much 😁

@BenjaminRodenberg BenjaminRodenberg changed the title Try to remove SolverInterfaceImpl::mesh() Try to remove ParticipantImpl::mesh(const std::string &meshName) Jun 23, 2023
@BenjaminRodenberg
Copy link
Member Author

BenjaminRodenberg commented Jun 23, 2023

I updated SolverInterfaceImpl to ParticipantImpl following #1643

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintainability Working on this will make our lives easier in the long run as preCICE gets easier to maintain. question Everybody is invited to answer this question or give any hint.
Projects
None yet
Development

No branches or pull requests

2 participants