idaholab / moose

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

Improve error message for PolygonConcentricCircleMeshGenerator #27979

Open lewisgross1296 opened 2 months ago

lewisgross1296 commented 2 months ago

Motivation

I spent an embarrassing amount of time trying to figure out why my block used in two mesh generation scripts

  [fuel_compact]
    type = PolygonConcentricCircleMeshGenerator
    num_sides = 6
    polygon_size = ${fparse pin_lattice_pitch/2}
    ring_radii = '${fparse 0.6*fuel_compact_radius} ${fparse 0.8*fuel_compact_radius} ${fuel_compact_radius}'
    ring_intervals = '2 2 2'
    num_sectors_per_side = '6 6 6 6 6 6'
    ring_block_ids = '2 2 2'
    ring_block_names = 'fuel fuel fuel'
    background_block_ids = '1'
    background_block_names = 'graphite'
    background_intervals = 5
  []

was failing in one and succeeding in another via this error message


*** ERROR ***
solid_mesh.i:17: (Mesh/fuel_compact/ring_block_ids):
    This parameter must have the appropriate size if it is provided. The size should be the same as the size
    of 'ring_intervals' if the innermost ring interval (including boundary layers) is unity; otherwise the 
    size should  be greater than the size of 'ring_intervals' by one. If 'quad_center_elements' is true, 
    it is optional to only provide this parameter with the same size as 'ring_intervals'

Turns out it was because I forgot to copy over

[GlobalParams]
  quad_center_elements = true
[]

Design

Looking at modules/reactor/src/meshgenerators/PolygonConcentricCircleMeshGeneratorBase.C, the error messages are pretty verbose and could probably be improved with some more case checking and printing out the specific case which caused the error, instead of printing all the possible issues.

E.g. here, it could have figured out that quad_center_elements was not set and print something like

Expecting a ring_block_ids with length of 4, since ring_intervals has a length of 3.
Either add one more block id or set quad_center_elements = true

Impact

Users will spend less time debugging their mesh generation script

kyriv1980 commented 2 weeks ago

In the documentation of this class, https://mooseframework.inl.gov/source/meshgenerators/PolygonConcentricCircleMeshGenerator.html we read :

The user should be aware that in two cases, the central geometric region is defined as two blocks instead of one: - When rings are not present, and "background_intervals" > 1 or a boundary layer is defined in this region - When rings are present, and the first entry of "ring_intervals" > 1 or a boundary layer is defined in this region

In both of these cases, the central geometric region will contain both blocks 0 and 1 by default. Any additional usage of block ids or name arrays will require an extra entry for the first geometric region. This extra block definition is needed to accommodate flexibility in defining the central region as either quadrilateral or triangular elements while still stitching properly to the neighboring region. Note that if "quad_center_elements" is set as true, the central geometric region is allowed to have a uniform block id/name. This can be achieved by providing a pair of duplicated block ids/names in the corresponding customized block ids/names input parameter. The user can also provide a single customized block id/name for the central geometric region in this case and the system will duplicate the input for the user.

In your example it's the fact that first element in (ring_block_ids = '2 2 2') is >1 and quad_center_elements is by default false, that causes a problem.

It may not seem like it but i think that the current error message is spot on. Maybe we could re-phrase the last sentence to read:

If 'quad_center_elements' is true the size of 'ring_block_ids' can be equal to sizeof(ring_intervals) or sizeof(ring_intervals) + 1.

lewisgross1296 commented 5 days ago

I see what you are saying. The last sentence does read better as you wrote it.

I guess my complaint is more that having this

If 'quad_center_elements' is true, it is optional to only provide this parameter with the same size as 'ring_intervals'

in the error message puts more work on the user to parse an already long and technical message, when instead we can just check the value of quad_center_elements.

If quad_center_elements = true, and the numbers are still invalid, then we can say what the size of ring_block_ids is and what it is allowed to be.

If it is false, and it would be okay if it were true, we can report that too.