search

prisms-center / phaseField

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

Fixed FloodFiller unit test and fixed grainGrowth bug #224

Closed zachcroft closed 2 months ago

zachcroft commented 2 months ago

Fixing the issue #219

1) Unit tests for FloodFiller and OrderParameterRemapper (which calls FloodFiller) were failing because of a modification that assigns the most common value of nodes to ele_val. The unit test has a grain with only one nodal value, so this wasn't being detected since the most common value would be null. I added a conditional that the most common value must be nonzero.

2) Because of the change to how ele_val was calculated, the threshold check for assigning vertices to grains also changed. Inconsistencies in grain vertex lists causes them to give rise to the "cracked" appearance described in the issue. The issue wasn't present in parallel because mergeSplitGrains() is called. To fix this issue for serial runs, I added a call to mergeSplitGrains() in the calcGrainSets function.

landinjm commented 2 months ago

The minimum grain id is hard-coded to zero, so it's fine as is.

I do think we should revisit the unit tests in the future to more thoroughly cover examples of grain reassignment (both different microstructures & something like 3-5 rectangular grains in a row)

landinjm commented 2 months ago

Per our meeting, keep the part with the mergeSplitGrains() and remove the part with the thresholding. Instead, change the unit test.

zachcroft commented 2 months ago

The minimum grain id is hard-coded to zero, so it's fine as is.

I do think we should revisit the unit tests in the future to more thoroughly cover examples of grain reassignment (both different microstructures & something like 3-5 rectangular grains in a row)

I want to make a comment that my most recent commit modified the ele_val threshold to be min_id instead of zero, but there is no technical justification for this change and it possibly introduced a bug.

I agree unit tests should be revisited. In my opinion, the queue flood fill algorithm needs to be implemented since it creates a large speed-up (I will submit a PR in the near future). We can create a new unit test for this algorithm.