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

Update AutoPas to Combined Tuning #270

Merged
merged 35 commits into from
Apr 16, 2024
Merged

Conversation

FG-TUM
Copy link
Member

@FG-TUM FG-TUM commented Jul 28, 2023

Description

Makes the necessary changes to support the new AutoPas version.

New AutoPas Features

  • Rule-based Tuning
  • Combination of tuning strategies
  • Support for multi-site particles (not supported with ls1 yet!)
  • Cleaner separation of library and application code

Fixed Bugs

  • Duplication of particles moving from one MPI rank to another.

TODO

How Has This Been Tested?

  • Simulations

@FG-TUM FG-TUM added the AutoPas label Jul 28, 2023
@FG-TUM FG-TUM self-assigned this Jul 28, 2023
@FG-TUM FG-TUM marked this pull request as ready for review January 16, 2024 11:21
SamNewcome
SamNewcome previously approved these changes Jan 16, 2024
Copy link
Contributor

@SamNewcome SamNewcome left a comment

Choose a reason for hiding this comment

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

All good.

Can you elaborate on what you meant by " freeze AutoPas version when Less Halo Halo Interactions AutoPas/AutoPas#787 is merged."

But otherwise all good for merge from my side.

examples/all-options.xml Outdated Show resolved Hide resolved
@FG-TUM
Copy link
Member Author

FG-TUM commented Jan 16, 2024

All good.

Can you elaborate on what you meant by " freeze AutoPas version when Less Halo Halo Interactions AutoPas/AutoPas#787 is merged."

But otherwise all good for merge from my side.

This refers to the default AutoPas tag in autopas.cmake. During development of the branch it was set to some AutoPas branch name, so the actual version was changing when the branch was updated. Since this means we can accidentally break ls1 by updating AutoPas, the default tag in the master of ls1 should always be one specific commit.

@SamNewcome
Copy link
Contributor

All good.
Can you elaborate on what you meant by " freeze AutoPas version when Less Halo Halo Interactions AutoPas/AutoPas#787 is merged."
But otherwise all good for merge from my side.

This refers to the default AutoPas tag in autopas.cmake. During development of the branch it was set to some AutoPas branch name, so the actual version was changing when the branch was updated. Since this means we can accidentally break ls1 by updating AutoPas, the default tag in the master of ls1 should always be one specific commit.

Okay makes total sense

@FG-TUM
Copy link
Member Author

FG-TUM commented Jan 29, 2024

How I analyzed the duplication bug:

  1. See CI fail
    -> rerun failing scenario locally.
  2. Observe regular stdout
    -> see insane spike in energy
    -> assumption: two particles came too close
  3. Look at ParaView
    • Identify a problematic particle
      -> color particles by force magnitude
      -> set color scale to include transparency
      -> only particles with super high forces visible
    • Get information about this particle
      -> activate "Hover on points" to see e.g. particle ID
      -> color by rank to see between which ranks it moves
      -> add pathlines to better understand the trajectory
  4. Understand what happens in the code
    -> add print statements (with #pragma omp critical) to check which rank has the particle ID in question at which point
    -> use this information to understand in which step the duplication happens
    -> draw a big diagram to understand the convoluted mess of ls1
    -> run the scenario with gdb with breakpoints in questionable places
    -> step through questionable code locations
  5. As soon as I narrowed it down to have a rough idea what the problem is:
    -> change the code to do things simpler even if less efficient to see if this fixes things and thus understand the nature of the bug better
    -> use the debugger line by line until I exactly understand what goes wrong
  6. Write a unit test that reproduces the bug in a minimal environment.
    -> helps to understand the edge cases better
    -> prevents the bug from reappearing in the future
  7. Fix the code until the test passes.
  8. Confirm it also fixes the original scenario.

@FG-TUM FG-TUM requested a review from SamNewcome February 14, 2024 15:22
@amartyads
Copy link
Contributor

amartyads commented Mar 19, 2024

Testing with this branch, and with the autopas version set to master, my earlier bug with region iterators is not occurring anymore. Particles in the ghost layers seem to be correctly deleted and not skipped. (issue #281 )

SamNewcome
SamNewcome previously approved these changes Mar 21, 2024
@FG-TUM FG-TUM requested a review from SamNewcome April 12, 2024 15:09
@SamNewcome SamNewcome merged commit 64002ac into master Apr 16, 2024
52 checks passed
@FG-TUM FG-TUM deleted the updateAutoPasCombinedTuning branch May 2, 2024 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants