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

Fixed FloodFiller unit test and fixed grainGrowth bug #224

Merged

Conversation

zachcroft
Copy link
Contributor

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 landinjm linked an issue Aug 30, 2024 that may be closed by this pull request
@landinjm landinjm mentioned this pull request Sep 4, 2024
@landinjm
Copy link
Contributor

landinjm commented Sep 4, 2024

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
Copy link
Contributor

landinjm commented Sep 4, 2024

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

@landinjm landinjm merged commit 40eb0f4 into prisms-center:development Sep 4, 2024
2 checks passed
@zachcroft
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug with default grainGrowth application during grain reassignment
2 participants