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

Fix velocity assignment in CubicGridGenerator (and others) #377

Merged
merged 4 commits into from
Feb 4, 2025

Conversation

HomesGH
Copy link
Contributor

@HomesGH HomesGH commented Jan 30, 2025

Description

As described in issue #376, the CubicGridGenerator uses the initCubicGrid method which uses the getRandomVelocity method.
The latter method can only handle particles with mass=1 correctly. This is fixed with the present PR by using the EqualVelocityAssigner to get the random initial velocities.

In addition, the CubicGridGenerator removes the overall drift, which is very beneficial. Unfortunately, this is not done properly as the velocities need to be scaled afterwards again to match the desired temperature. This was also fixed in the present PR.

Resolved Issues

How Has This Been Tested?

  • example DropletCoalescence/liq/config_1_generateLiq.xml was run and the initial temperatures where checked. Everthing looks good now and the initial temperatures are now the target temperatures. Without the rescaling, they would be too low and without considering the mass, the temperatures would be completely off (explosion)

Further improvements (not part of the present PR)

In the future, it would be nice

  • to add a mechanism which ensures that the desired density is met, e.g. by deleting particles. Currently, there is no generator that just creates a simulation with a given density and temperature (like in ms2).
  • to add the possibility to select the velocity assigner to be used, see comment below.
  • to clean up the class, e.g. get rid of "Internal" in names and move initCubicGrid from the particle container to the generator/helper class.

@HomesGH HomesGH added the bug Something isn't working label Jan 30, 2025
@HomesGH HomesGH marked this pull request as ready for review January 31, 2025 13:00
@amartyads
Copy link
Contributor

Thanks for looking into this bug, it's been a pain for a very long time! I can review in a few days, but a few initial thoughts:

  1. Having this GridGenerator in the io folder along with a bunch of output plugins is very confusing imo, especially with a separate generators folder, maybe we should have separate input and output folders? In a different PR?
  2. If we want to use velocity assigners, maybe it's better to use the approach in ObjectGenerator.cpp, and read in the xml tag <velocityAssigner>, so that we can use MaxwellVelocityAssigner, as well as the random seed features, instead of using only EqualVelocityAssigner. We can create the velocityAssigner object in CubicGridGenerator and pass it to IOHelpers.

@HomesGH
Copy link
Contributor Author

HomesGH commented Jan 31, 2025

Thanks for looking into this bug, it's been a pain for a very long time! I can review in a few days, but a few initial thoughts:

1. Having this GridGenerator in the `io` folder along with a bunch of output plugins is very confusing imo, especially with a separate `generators` folder, maybe we should have separate `input` and `output` folders? In a different PR?

2. If we want to use velocity assigners, maybe it's better to use the approach in `ObjectGenerator.cpp`, and read in the xml tag `<velocityAssigner>`, so that we can use `MaxwellVelocityAssigner`, as well as the random seed features, instead of using only `EqualVelocityAssigner`. We can create the `velocityAssigner` object in `CubicGridGenerator` and pass it to `IOHelpers`.

Thank you for your review!

  1. Yes, this definitely needs a clean-up! There is also still the "Internal" in the file name and it uses a method of the particle container to create the grid, which should be moved to the generator itself in my opinion. However, this might be done in another PR.
  2. That's a good idea! But for now, I just want to make it work (somehow) so that I can continue with the other PRs, e.g. the validation (CI) which currently fails due to the one example with a bad grid setup. Similar to 1., I would prefer doing this in another PR.
    I have added those two points to the above description as possible improvements in the future.

@cniethammer
Copy link
Contributor

I am not super deep into this part of the code and just had the chance for a very rough look.
Overall this looks good to me. I like that you reused and re-checked my code for the velocity assignement from the utils generator library.

@HomesGH HomesGH merged commit e48af35 into master Feb 4, 2025
52 checks passed
@HomesGH HomesGH deleted the fixCubicGridGenerator branch February 4, 2025 10:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong temperature assigned in CubicGridGenerator
3 participants