prisms-center / phaseField

PRISMS-PF: An Open-Source Phase-Field Modeling Framework
https://prisms-center.github.io/phaseField/
Other
213 stars 117 forks source link

Bug with default grainGrowth application during grain reassignment #219

Closed zachcroft closed 1 week ago

zachcroft commented 3 weeks ago

The default grainGrowth app contains a bugged order parameter after the first grain reassignment step (t=100). The bug only occurs when run in serial, but doesn't occur for a parallel simulation.

grainGrowth

I determined the bug occurs due to the following commit (reverting the changes in the commit fixes the bug). https://github.com/prisms-center/phaseField/commit/6e91b505361ab09e39029b8801a2f1f304ad572a

The bug also causes 2 unit tests to fail, as described in the following post. Reverting the commit above fixes this issue. https://github.com/prisms-center/phaseField/pull/203

Attached are output files to compare serial and parallel runs. grain_growth_serial.txt grain_growth_2threads.txt

landinjm commented 3 weeks ago

Why is this new method preferred for grain remapping?

zachcroft commented 3 weeks ago

Why is this new method preferred for grain remapping?

I'm not sure, there was no explanation in the commit. I suggest we just go with Jacob's implementation of the recursiveFloodFill() function since it provides significant speedup. I don't know if it's been testing/documented, I will look into it and submit a pull request when it's ready.

david-montiel-t commented 3 weeks ago

If I remember correctly, the commit was meant fixed grain detection. I'm fine with going with recursiveFloodFill but I would like to fix (or at least identify the cause) of the bug in the old method first.

On Fri, Aug 23, 2024, 2:22 PM zachcroft @.***> wrote:

Why is this new method preferred for grain remapping?

I'm not sure, there was no explanation in the commit. I suggest we just go with Jacob's implementation of the recursiveFloodFill() function since it provides significant speedup. I don't know if it's been testing/documented, I will look into it and submit a pull request when it's ready.

— Reply to this email directly, view it on GitHub https://github.com/prisms-center/phaseField/issues/219#issuecomment-2307591431, or unsubscribe https://github.com/notifications/unsubscribe-auth/AD3FA4ZPXMB5SOA4ABYBUUTZS54VRAVCNFSM6AAAAABM2KUQPWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMBXGU4TCNBTGE . You are receiving this because you are subscribed to this thread.Message ID: @.***>

landinjm commented 3 weeks ago

This bug persists with 1st order element and uniform mesh.

david-montiel-t commented 3 weeks ago

To reiterate (I've checked now), the commit is unrelated to the grain remapping method.

The only thing I can think of is that the new grain detection method (which avoids identifying some grain boundaries as grains) is slightly modifying the simplified grain representation.

This simplified grain representation is what is used for grain remapping. What is strange is that the issue only happens in series and not in parallel.

On Fri, Aug 23, 2024 at 3:17 PM Jason Landini @.***> wrote:

This bug persists with 1st order element and uniform mesh.

— Reply to this email directly, view it on GitHub https://github.com/prisms-center/phaseField/issues/219#issuecomment-2307674079, or unsubscribe https://github.com/notifications/unsubscribe-auth/AD3FA4643LKK25IAVJHHYCDZS6DEDAVCNFSM6AAAAABM2KUQPWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMBXGY3TIMBXHE . You are receiving this because you commented.Message ID: @.***>

david-montiel-t commented 3 weeks ago

What is also strange is that the FloorFiller method is supposed to apply only to identify grains loaded from an artificial microstructure (e.g., from DREAM.3D) which is not the case in the grainGrowth app.

zachcroft commented 3 weeks ago

What is also strange is that the FloorFiller method is supposed to apply only to identify grains loaded from an artificial microstructure (e.g., from DREAM.3D) which is not the case in the grainGrowth app.

I believe the flood fill algorithm is used for grain reassignment.

stvdwtt commented 3 weeks ago

@david-montiel-t, I don't think that's true. If I remember correctly (which maybe I don't...) there's a flood fill operation every time there's an order parameter remapping. That's how the code knows where grains are and can cut them out to move them to new order parameters.

Separately -- I'm glad to see that this portion of the code is getting some use!

david-montiel-t commented 3 weeks ago

You are right, Steve. I was mistaken. The modification was made originally to improve grain detection from DREAM.3D but we did not think about the effect on identifying the grains during remapping.

zachcroft commented 2 weeks ago

I've made some progress with this bug and why the commit I mentioned causes the FloodFiller and OrderParameterRemapper unit tests to fail.

FloodFiller unit test: This test seems to initialize a 5x5 mesh with two 'grains' initialized with a value of 1 at the top left and bottom right corners (pictured).

image

The commit https://github.com/prisms-center/phaseField/commit/6e91b505361ab09e39029b8801a2f1f304ad572a contains a modification so that the element value (ele_val) is determined by the most frequent value of its nodes. Since the top left grain is defined as only a single point, the most common value of the top left cell will be zero since there are 3 zeros and a single 1, resulting in the grain not being detected and the unit test failing. The OrderParameterRemapper unit test makes a call to FloodFiller so that test will also fail.

To fix this, I suggest adding a conditional that the most common value must be greater than zero, or whatever the minimum id value is.

Fixing the bug: I found that increasing the order parameter cutoff for grain identification in parameters.prm gets rid of the "cracked" grain issue I originally mentioned, but there's still the question of why this bug only occurs for serial runs. I confirmed that this is due to the mergeSplitGrains() function in FloodFiller.cc not being called in serial, which combines grains if they share common vertices. Also note that the vertex list for a grain is only populated if it meets the threshold criteria.

My suggestions:

  1. Either revert the ele_val calculation or add a conditional to make sure the most common value is valid (fixes the unit test)
  2. Call mergeSplitGrains() during every call to FloodFiller (fixes the original bug)

It might also be a good idea to write/modify the unit tests for testing different scenarios/microstructures. Please let me know if you have any concerns about these suggestions.