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.

Sign up for GitHub

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

Jump to bottom

tentative fix to ALL exit bug #245

Closed
wants to merge 2 commits into from
Closed

tentative fix to ALL exit bug #245

wants to merge 2 commits into from

Conversation

amartyads
Copy link
Contributor

@amartyads amartyads commented Oct 24, 2022 •
edited by FG-TUM
Loading

Description

Simulation now exits gracefully when ALL update frequency < number of simsteps

Resolved Issues

How Has This Been Tested?

Same test case as outlined in issue

@amartyads amartyads requested a review from FG-TUM October 24, 2022 18:33
@amartyads amartyads changed the title tentative fix to bug tentative fix to ALL exit bug Oct 24, 2022
FG-TUM
FG-TUM reviewed Oct 25, 2022
View reviewed changes
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.

Just a comment: I find it a bit unintuitive to pass the number of timesteps to the constructor because why should the decomposition actually care about that info (yes for the ALL bugfix but actually it shouldn't). The other way to get to the information of the number of timesteps would be by querying the xmlconfig during the readXML() step. However, I suppose then you would miss the number of steps when it is passed via the command line. For this you could query global_simulation->getNumTimesteps(). On the other hand, accessing the singleton is always an ugly workaround, but already widely used throughout ls1. So maybe an ok-ish solution in readXML() could be

  1. check if global_simulation != nullptr and query that if available
  2. otherwise query xmlconfig.

In the end, I feel all of the options are ugly in their own way, so decide for yourself what you prefer.

@cniethammer
Copy link
Contributor

I agree with@FG-TUM that the domain decomposition should not require the number of time steps to simulate. Note also that ls1 could be run using a time limit instead a fixed number of steps.
I did not follow the details of the problem with ALL(?) and Autopas here but would think that there should be more solutions.
However, if no other solution is there, for me using the singleton seems like the way to go. Reading other parts of the xml that are not part of the <decomposition> tags in my opinion as duplicates I/O logic that is error prone and hence a bad thing IMHO.

@amartyads
Copy link
Contributor Author

I agree that all workarounds are ugly. I personally wanted to avoid accessing the singleton since I'd like to be able to phase out its use in the future. I'll take a look at the initialization procedure for the ALL object and see if I can fix it in a more logical way, however that will take some time.

@FG-TUM
Copy link
Member

FG-TUM commented Oct 25, 2022

Note also that ls1 could be run using a time limit instead a fixed number of steps.

@cniethammer Does that feature (still) exist? It is not mentioned in all-options.xml. How does it work?

@cniethammer
Copy link
Contributor

Note also that ls1 could be run using a time limit instead a fixed number of steps.

@cniethammer Does that feature (still) exist? It is not mentioned in all-options.xml. How does it work?

From a quick look into the source I see that, e.g., the --timed-checkpoint is still present that was used for PBS/Slum jobs with limited runtime in the past.

@amartyads
Copy link
Contributor Author

@FG-TUM I've added some more details to the issue #244 , since I discovered the root cause and it made more sense to put it there

@FG-TUM
Copy link
Member

FG-TUM commented May 24, 2023

These changes do not fix the actual bug -> closing

@FG-TUM FG-TUM closed this May 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FG-TUM FG-TUM FG-TUM left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ALL + AutoPas exit error when ALL updateFrequency < total number of sim steps
3 participants
@amartyads @cniethammer @FG-TUM