idaholab / moose

Multiphysics Object Oriented Simulation Environment
https://www.mooseframework.org
GNU Lesser General Public License v2.1
1.78k stars 1.05k forks source link

Rename more inconsistent variables in phase_field (ID vs Index) #7638

Open permcody opened 8 years ago

permcody commented 8 years ago

Description of the enhancement or error report

The grain tracker has been taking the blame for awhile now with getting different IDs than the EBSDReaderPointDataAux object. Well after working on the test problem for awhile I was convinced there was a problem because I could tell just by looking at the file that the AuxKernel version was the correct version. Turns out the bug is in the Functor stuff somewhere.

Rationale for the enhancement or information for reproducing the error

The latest PR is reading EBSD data directly instead of using the variables set from either ReconVarIC. I had originally copied the logic from ReconVarICinto the GrainTracker but got the same answer as ReconVarIC (nothing unexpected there). However, I decided to duplicate the logic in EBSDReaderPointDataAux inside of the GrainTracker and low and behold, the value in the output is correct!

To be perfectly clear these two methods of obtaining the grain_index give different answers!!!

// Method in `GrainTracker` and `ReconVarIC` incorrect on existing grain_tracker_ebsd.i
    Point centroid = elem->centroid();
    const EBSDAccessFunctors::EBSDPointData & d = _ebsd_reader->getData(centroid[0]);
    const unsigned int global_index = _ebsd_reader->getGlobalID(d._feature_id);

// Method in `EBSDReaderPointDataAux correct on existing grain_tracker_ebsd.i
   MooseEnum field_types = EBSDAccessFunctors::getPointDataFieldType();
   field_types = "feature_id";
   MooseSharedPointer<EBSDAccessFunctors::EBSDPointDataFunctor> 
      val(_ebsd_reader->getPointDataAccessFunctor(field_types)
   const unsigned int global_index = (*val)(_ebsd_reader->getData(centroid[0]));

@dschwen - what's up?

Identified impact

(i.e. Internal object changes, limited interface changes, public API change, or a list of specific applications impacted)

Towards fixing broken EBSD tests Tag @tonkmr, @frombs

permcody commented 8 years ago

I've been working with @dschwen all morning and discovered that this is actually OK and the problem I'm seeing comes from mixed terminology again. The GrainTracker is using global_id and local_id throughout while the value we are viewing in these EBSD examples is actually feature_id. To be clear, there are differences between the feature_id field and the global_id field!

I'm going to propose yet another naming change to the GrainTracker and all of this EBSD logic to further simply usage moving forward. I think we really need to differentiate between which names signify something that can be used as an index into a vector, and something that may be non-contiguous. To that end, I suggest the following changes to the names currently in use.

 *  * feature_id        The grain number in the EBSD data file.
 *  * global_index    The index into the global average data (contiguously numbered)
 *                             This is also called "(global) grain index"
 *  * local_index      The index into the per-phase grain list. (contiguously numbered and only
 *                             unique when combined with phase number)
 *                             This is also called "(local) grain index"

This would mean changes to the grain tracker as well. Instead of UNIQUE_REGIONS in the FeatureFloodCountAux, I'll be renaming that field to UNIQUE_INDEX or something along those lines (index being the key word here).

Comments?