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

Replace Simulation::exit with mardyn_exit #334

Merged
merged 13 commits into from
Jul 30, 2024
Merged

Replace Simulation::exit with mardyn_exit #334

merged 13 commits into from
Jul 30, 2024

Conversation

HomesGH
Copy link
Contributor

@HomesGH HomesGH commented Jul 26, 2024

Description

Simulation.h is included in many files and can easily lead to some chaos due to circular dependency (as @FG-TUM mentioned).
Therefore, this PR replaces the Simulation::exit() method with mardyn_exit() since this only requires the include of mardyn_assert.
The Simulation::exit() method is also deleted.

During the replacement, I paid attention to no include the Simulation.h in any header file (exception: Planar.cpp since it requires Simulation as one argument of its methods solved by class Simulation; instead if #include). Therefore, I had to move Domain::setComponentThermostat from the header to the cpp file.

What became apparent during the replacement:
A lot of files use global_simulation or _simulation, e.g. to get the ensemble.

Copy link
Member

@FG-TUM FG-TUM left a comment

Choose a reason for hiding this comment

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

A lot of files now got a new #include "Simulation.h". Was this on purpose?

src/plugins/Dropaccelerator.cpp Show resolved Hide resolved
src/parallel/ForceHelper.h Show resolved Hide resolved
src/Domain.cpp Show resolved Hide resolved
FG-TUM
FG-TUM previously approved these changes Jul 26, 2024
@FG-TUM FG-TUM added the clean-up related to the clean-up of the code and tech dept label Jul 26, 2024
src/Simulation.cpp Dismissed Show resolved Hide resolved
@HomesGH HomesGH mentioned this pull request Jul 26, 2024
9 tasks
@HomesGH HomesGH requested a review from FG-TUM July 26, 2024 18:31
FG-TUM
FG-TUM previously approved these changes Jul 29, 2024
Copy link
Member

@FG-TUM FG-TUM left a comment

Choose a reason for hiding this comment

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

I'd prefer a bit more of a "let it crash and burn" philosophy, meaning, that if something is misconfigured I'd rather abort a simulation instead of continuing with a different setting. Usually when we configure something we do it for a reason and don't expect the code to do something different which we would only realize after inspecting the logs. A crash error message would be clearer.

Apart from that I'm fine with the changes!

src/Simulation.cpp Dismissed Show resolved Hide resolved
src/molecules/AutoPasSimpleMolecule.cpp Show resolved Hide resolved
@HomesGH
Copy link
Contributor Author

HomesGH commented Jul 29, 2024

Ok, it’s actually AutoPas which breaks the CI…

@FG-TUM
Copy link
Member

FG-TUM commented Jul 29, 2024

Ok, it’s actually AutoPas which breaks the CI…

This is what I meant. So the clean thing would be to adapt the tests. But this is ls1, so I personally could also live with going back to the previous philosophy of (almost) silent changes in behavior and cheating the tests just for the sake of getting the rest of this PR towards master...

@HomesGH HomesGH requested a review from FG-TUM July 30, 2024 09:05
@FG-TUM FG-TUM merged commit c87bd23 into master Jul 30, 2024
52 checks passed
@HomesGH HomesGH deleted the properexit branch July 30, 2024 09:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clean-up related to the clean-up of the code and tech dept
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants