Adding fix and tests for spherical mesh as spatial distribution

[pshriwise]

Description

This passes the cosine of theta bound for a spherical mesh bin to uniform_distribution instead of the angles themselves. It also adds tests using regular, cylindrical, and spherical meshes with MeshSpatial with some checks to ensure we're getting valid source values. To simplify said tests I've also added a SphericalMesh.from_domain classmethod.

Small additional change: the origin attribute is now written as part of the SphericalMesh.__repr__.

Fixes #3427

Checklist

• I have performed a self-review of my own code
• I have run clang-format (version 15) on any C++ source files (if applicable)
• I have followed the style guidelines for Python source files (if applicable)
• I have made corresponding changes to the documentation (if applicable)
• I have added tests that prove my fix is effective or that my feature works (if applicable)

[gonuke]

Thanks for this quick fix @pshriwise - Have a couple of implementation quetions?

[pshriwise]

Like a vine on the wall, the scope has crept after thinking over some of the discussion above.

I noticed that there were some issues with the CylindricalMesh.from_domain implementation causing it to set the radius of the mesh as if the mesh were centered on the origin, but the origin is also set, so some fixes for that are now included. I've updated the method expected test results to reflect a mesh that is translated into place and matches the size of the bounding box.

I've also added an inscribed attribute to these methods that will indicate whether or not the mesh should inscribe or circumscribe the bounding box of the provided domain. This isn't quite an accurate term because we'd need an elliptic mesh to properly create an inscribed mesh for non-cubic bounding boxes. I'll think on a better argument name.

I also now need to add tests for inscribe=True. This is my own fault and I accept it.

[pshriwise]

I should also note that these updates do change the behavior of the CylindricalMesh.from_domain method. Previously the maximum of the x and y dimensions was used to set the mesh's outer mesh. Now, in the case that the mesh is set to enclose the bounding box, the distance from the center of the x-y face to the corner of the x-y face is used.

[gonuke]

A couple of last questions...

[pshriwise]

I opted for enclose_domain over inscribe as it's more descriptive.

[gonuke]

I opted for enclose_domain over inscribe as it's more descriptive.

(I don't see that change yet, FYI...)

[gonuke]

This looks good to me. Thanks @pshriwise. Hopefully the tests will pass and it can be merged soon

[paulromano]

Thanks @pshriwise. I just made one update, which is to introduce a HasBoundingBox protocol for the domain so that if there is some future type with a bounding_box attribute, we don't need to change the type annotation to account for it.

One other thought -- instead of having arguments like name, mesh_id, phi_grid_bounds, etc. in from_domain that just get passed on to the normal constructor, we could just take **kwargs that get passed. That way if the constructor changes, we don't need to update it in two places. What do you think of that?

[pshriwise]

Thanks @pshriwise. I just made one update, which is to introduce a HasBoundingBox protocol for the domain so that if there is some future type with a bounding_box attribute, we don't need to change the type annotation to account for it.

I like it! Good call.

One other thought -- instead of having arguments like name, mesh_id, phi_grid_bounds, etc. in from_domain that just get passed on to the normal constructor, we could just take **kwargs that get passed. That way if the constructor changes, we don't need to update it in two places. What do you think of that?

I like this as well. 👍🏻 I'm going to vote for that happening in a separate PR though as we haven't had too much call for it and this PR has crept a fair amount in scope already.

[paulromano]

@pshriwise Are you OK with me taking a stab at the **kwargs update as part of this PR?

[pshriwise]

@pshriwise Are you OK with me taking a stab at the **kwargs update as part of this PR?

I guess I'd prefer we cut this one off where it is an start a new one. The scope crept a fair amount already (mainly to make test patterns consistent with other mesh classes) so I think a further refactor like that would make sense as a separate PR.