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 bug in GridFiller when using <density> option #375

Closed
wants to merge 4 commits into from

Conversation

HomesGH
Copy link
Contributor

@HomesGH HomesGH commented Jan 29, 2025

Description

Currently, the GridFiller might place particles in a way that leads to explosions at the beginning of the simulation. This is due to the close distance of some particles across the periodic boundary conditions when the entire simulation domain should be filled with a grid. See also issues below.
The problem is, in my opinion, that the size of the unit cell is calculated with the desired density. However, the desired density is probably not met at the end due to the lattice and therefore the discrete number of inserted particles. Another issue could be that a cubic unit cell is used.
This PR solves this problem by scaling the unit cell so that the object box size is evenly divisible by the unit cell size. This leads to the same distance between all unit cells even across the PBC.

This PR also cleans up the readXML method of the GridFiller since (as already written in the doc) the <lattice> option is ignored/overwritten when the <density> option is set. Therefore, <lattice> is only read if <density> was not specified.
This is also the reason why <lattice> was deleted in the config xmls in files where <density> was set.

Further improvements

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).
This is not part of the present PR.

Resolved Issues

How Has This Been Tested?

There are explosions when running this example, see also PR #371.
With the changes of the present PR, the explosions are gone. It was also tested what happens when both <lattice> and <density> are specified (--> <lattice> is ignored as expected).

@HomesGH HomesGH changed the title Fix bug in Gridiller Fix bug in GridFiller when using density option Jan 29, 2025
@HomesGH HomesGH changed the title Fix bug in GridFiller when using density option Fix bug in GridFiller when using <density> option Jan 30, 2025
@HomesGH HomesGH requested a review from cniethammer January 30, 2025 08:09
@HomesGH
Copy link
Contributor Author

HomesGH commented Jan 30, 2025

Currently, the CI fails since the DropletCoalescence/liq/config_1_generateLiq.xml example fails. This is due to the fixed bug in GridFiller (and therefore, no explosions any more) and an issue also mentioned in PR #319. This should be solved with PR #371 (which fails due to the bug in the GridFiller). So I would suggest to comment out this example in the example list for the present PR and re-introduce it with PR #371.

@cniethammer
Copy link
Contributor

I see the issue you have.

This is indeed a limitation of the generator utilities library. From my point of view, this has to be solved in the io/Readers.
The idea of the GridFiller utility library has been that it can be used with any geometry and is not specialized for cubic objects.
Therefore, the adaptation of lattice parameters should not be made here as you may get very strange results for unifications or cuts of objects.

@HomesGH
Copy link
Contributor Author

HomesGH commented Jan 30, 2025

How or where would you solve this in the io/Readers? After the generation of the initial particle configuration, the is no reader/IO involved before the simulation starts. Why should the reader or IO modify the position of the particles?

@cniethammer
Copy link
Contributor

It has been some time since I implemented all of this and I need to have a look at the details again.
As stated in my PhD thesis the problem of exact density can be solved using Robert Floyd’s algorithm - which I did not find time to implement there as the main goal was to achieve generalisation and scalability for runs with 100.000+ MPI processes.

@HomesGH HomesGH mentioned this pull request Jan 30, 2025
1 task
@HomesGH
Copy link
Contributor Author

HomesGH commented Jan 30, 2025

I see the issue you have.

This is indeed a limitation of the generator utilities library. From my point of view, this has to be solved in the io/Readers. The idea of the GridFiller utility library has been that it can be used with any geometry and is not specialized for cubic objects. Therefore, the adaptation of lattice parameters should not be made here as you may get very strange results for unifications or cuts of objects.

Ok. So for just setting up a simulation filled with a grid which should not explode, the use of the CubicGridGenerator is better. There were other issues with it (see #376), which I fixed in PR #377.
Then let's change the generator in the example DropletCoalescence/liq/config_1_generateLiq.xml to prevent it from explosions.

@HomesGH HomesGH closed this Feb 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Temperature distribution after using GridFiller Bug in GridFiller/ObjectGenerator
2 participants