Extra error checking on mesh creation

shimwell commented 1 month ago

Description

Currently it is possible to build cylindricalmesh objects with phi_grid values that don't make sense. For example

cylindrical_mesh = openmc.CylindricalMesh(
    r_grid=[0, 1, 2],
    phi_grid=[2],
    z_grid=[0, 10, 20],
)
>>> CylindricalMesh
        ID             =        9
        Name           =
        Dimensions     =        3
        Origin         =        [0. 0. 0.]
        N R pnts:      =        3
        R Min:         =        0.0
        R Max:         =        1.0
        N Phi pnts:    =        1
        Phi Min:       =        2.0   <----- min is the same as max
        Phi Max:       =        2.0   <----- min is the same as max
        N Z pnts:      =        4
        Z Min:         =        0.0
        Z Max:         =        20.0

Perhaps we could add some more checking on the setters to check that the min value and max value for phi are not the same and raise a ValueError if they are the same. I guess this is the same for r_grid and z_grid values and even for other mesh types.

Alternatives

We could leave the code as is, The error gets reported to the user when they run the openmc executable. I think reporting the error when running the simulation is less desirable and Ideally the user would find out earlier (when creating the mesh)

Compatibility

no API changes needed

chrwagne commented 4 weeks ago

Hello, I am a computer science student at the University of Michigan, and I am interested in attempting to resolve this issue. Could you assign it to me?

hsameer481 commented 4 weeks ago

Hi chrwagne, hope you are doing well! We had asked for a task through the OpenMC discussion forum a couple days ago and shimwell added this. I would recommend maybe messaging there for another task, or looking at the forum for guidance since that helped our group.

shimwell commented 4 weeks ago

Oh wow two volunteers, this doesn't happen very often. How about one looks into fixing this for the CylindricalMesh and the other for the RegularMesh. It will be interesting to see the solutions and we might all learn something from each other if there are different ways of implementing a solution

hsameer481 commented 4 weeks ago

Sounds good! Thanks for your guidance

chrwagne commented 4 weeks ago

Ok sounds like a plan! I can tackle the regular mesh

shimwell commented 4 weeks ago

Ah sorry I looked more carefully at the code and I should have SphericalMesh not RegularMesh.

For SphericalMesh the values like r_grid can currently be set to incorrect numbers like this

>>> import openmc
>>> openmc.SphericalMesh(r_grid=[1,1])
SphericalMesh
    ID             =    1
    Name           =    
    Dimensions     =    3
    Origin         =    [0. 0. 0.]
    N R pnts:      =    2
    R Min:         =    1.0  <------ this is the same value twice, should not be allowed
    R Max:         =    1.0  <------ this is the same value twice, should not be allowed
    N Theta pnts:  =    2
    Theta Min:     =    0.0
    Theta Max:     =    3.141592653589793
    N Phi pnts:    =    2
    Phi Min:       =    0.0
    Phi Max:       =    6.283185307179586

The other aspect I should be clear about is that the error in these meshes is only reported when the simulation is run. I think ideally it should be reported earlier when the user makes the mesh with the python API checking and reporting the error in the class setter. However there is a chance that we do this work and the pull request gets rejected by the reviewer.

openmc-dev / openmc