Ensure correct initialization of members for RegularMesh

[aprilnovak]

I'm familiarizing myself with the Mesh representations, and came across what I think is an unprotected logic chain that could lead to unintended behavior for users. Basically, the RegularMesh is provided with a lower_left_ coordinate, and either:

A) computes upper_right_ from a provided shape_ and width_
B) computes width_ from a provided shape_ and upper_right_

(a third option is to compute the shape_ from width_ and upper_right_, but you probably wouldn't have a perfect integer number of cells and it doesn't seem like this is even supported).

Therefore, shape_ must be provided, but currently no error is thrown if it's not provided, in which case shape_ and n_dimension_ are not set. You would also miss the check that the dimension is 1-D, 2-D, or 3-D.

This PR protects against the fringe case where the user doesn't set "dimension".

[pshriwise]

Good catch @aprilnovak! These updates look good to me.

[aprilnovak]

Hmm, now that I've done a bit more looking, if we merge this, then I don't think we'd ever get to the default shape_ provided for the Shannon entropy mesh:

mesh.cpp

  if (m->shape_.size() == 0) {
      // If the user did not specify how many mesh cells are to be used in
      // each direction, we automatically determine an appropriate number of
      // cells
      int n = std::ceil(std::pow(n_particles / 20.0, 1.0/3.0));
      m->shape_ = {n, n, n};
      m->n_dimension_ = 3;

      // Calculate width
      m->width_ = (m->upper_right_ - m->lower_left_) / m->shape_;
    }

But for non-Shannon-entropy meshes, we do want to throw an error if the user doesn't provide the dimension in the input file. What would be the best way to handle this?

[pshriwise]

But for non-Shannon-entropy meshes, we do want to throw an error if the user doesn't provide the dimension in the input file. What would be the best way to handle this?

Oof good point. I'm not entirely sure. I honestly don't have a good sense of how often this feature is used -- it's possible that we could simply remove it and document the heuristic we're using to generate that mesh shape, but it does seem like a convenient thing to keep around if possible.

If we do want to keep this capability around, I'd really like to maintain the behavior you've added where the RegularMesh object is required to be fully specified upon construction. To do that and maintain the functionality of the automatically generated Shannon entropy shape I'm wondering if perhaps the ID of the Shannon entropy mesh could be set before read_meshes is called and passed as an optional parameter to the read_meshes function from read_settings_xml. The detection of a missing shape parameter for entropy mesh could then be detected inside that function and automatically computed before constructing the RegularMesh object (realizing that this might require the addition of a new constructor for RegularMesh as I write this). The simulation::entropy_mesh parameter could then be verified and set after read_meshes.

Considerable restructuring involved, but I think it would do the trick.

[aprilnovak]

What about this approach?

Create a new global variable, entropy_mesh_id, and in RegularMesh's constructor, check if id_ matches entropy_mesh_id, in which case we can use the default heuristic if the user doesn't specify some of the input parameters.

This would avoid the creation of a new constructor so that read_meshes and anywhere else that RegularMesh's constructor is called is un-changed. Then, we could move the assignment of the entropy mesh ID before read_meshes and everything should work as normal ...

... except for the deprecated <entropy> approach, which sets an ID of 10000 instead of reading something from an XML node. The id_ member would never be set in Mesh::Mesh this way.

I think I have a rather clean way to do this as long as the <entropy> option is totally deprecated. Any idea on when that is planned?

[pshriwise]

I like it! Keeps things simple and no new constructors needed.

Not sure about when support for <entropy> is planned to be removed. Any thoughts on that @paulromano?

[paulromano]

I'm fine removing the support for <entropy> at this point. Regarding the heuristic that is used to calculate the number of mesh cells in each direction, I think an easier way to clean things up is to move that heuristic into the Python code. At the time we call Settings._create_entropy_mesh_subelement, we can check whether the used specified Mesh.dimension and if not, assign it based on the heuristic.

[aprilnovak]

Ok I think I have this moved onto the Python side correctly. Some comments/questions:

• There seemed to be an unrelated typo with setting the i_ufs_mesh in settings.cpp (it set it incorrectly to index_entropy_mesh). I corrected this.
• The heuristic calculation currently requires that the user set the lower_left on the entropy mesh to find the correct length of the tuple entropy_mesh.dimension. What is your preferred way for ensuring that the user didn't forget lower_left? Or, what if the user sets the number of particles after setting the entropy mesh parameters? You get an error message like the following if you set settings.particles after the entropy mesh creation.

Traceback (most recent call last):
  File "build_xml.py", line 79, in <module>
    settings.export_to_xml()
  File "/home/anovak/openmc/openmc/settings.py", line 1357, in export_to_xml
    self._create_entropy_mesh_subelement(root_element)
  File "/home/anovak/openmc/openmc/settings.py", line 933, in _create_entropy_mesh_subelement
    n = ceil((self.particles / 20.0)**(1.0 / 3.0))
TypeError: unsupported operand type(s) for /: 'NoneType' and 'float'

[aprilnovak]

Also, with regards to deprecation - I left a fatal_error in place of fully removing any trace that <entropy> was ever a valid option. If there's a different convention, let me know

[paulromano]

• The heuristic calculation currently requires that the user set the lower_left on the entropy mesh to find the correct length of the tuple entropy_mesh.dimension. What is your preferred way for ensuring that the user didn't forget lower_left? Or, what if the user sets the number of particles after setting the entropy mesh parameters? You get an error message like the following if you set settings.particles after the entropy mesh creation.

For lower_left, we actually assume that lower_left is always given -- if it isn't, the mesh won't be exported correctly. So, I'm not sure we need to do anything for this heuristic. If the number of particles is not set, we can throw an exception and tell the user they need to set it in order to set the entropy mesh accordingly.

[aprilnovak]

For lower_left, we actually assume that lower_left is always given

True, I forgot about that.

If the number of particles is not set, we can throw an exception

Good idea! Now included.

[pshriwise]

Re-approving as a formality after all of these additional improvements. 😃

[paulromano]

Thanks @aprilnovak!