Fix bug in cylindrical and spherical meshes when grid boundary is coincident with cell boundary

[paulromano]

A user recently noticed incorrect physical behavior when using a CylindricalMesh over a fuel pin, where the power profile should have exhibited a depression into the pin due to spatial self-shielding, but instead the opposite was observed. After digging into this I figured out that the problem was that if a cell boundary (in this case a z-cylinder) was exactly coincident with one of the mesh boundaries, the internal mesh raytrace routines would return an incorrect distance to the next grid boundary. In this PR, I've fixed this issue by explicitly checking whether the r position for a cylindrical or spherical mesh is coincident with the grid boundary, which is consistent with how we handle distance calculations for our geometry routines.

[pshriwise]

I think this is a step in the right direction! But there do appear to be some issues with the tally routines still. If you scale the geometry of these two tests up by a factor of 100, you'll see that the two tally means are significantly different.

I gave these changes a try for #2369 as well and while it did reduce the discontinuities @eepeterson was seeing, the tally plot still didn't look quite right. I'm working on a solution for that as well and will open a PR for it soon!

[paulromano]

Thanks for the careful review @pshriwise. So it looks like if I bump up the coincidence check constant (currently FP_COINCIDENT = 1e-12) to 1e-10, it seems to be a lot more robust. Namely, I took my test and adding a scaling factor per your suggestion and even when I take the scaling factor up to 1e7, the tests still pass. You might recall that I also ended up using a looser coindence tolerance when working on torii:

openmc/src/surface.cpp

Line 1047 in 7200d42

double cutoff = coincident ? 1e-10 : 0.0;

so bumping it up to 1e-10 here would be consistent. What do you think?

[paulromano]

@pshriwise I went ahead and loosened the tolerance on the coincidence check and also parameterized the tests to check different scales.

[pshriwise]

@pshriwise I went ahead and loosened the tolerance on the coincidence check and also parameterized the tests to check different scales.

Looks nice! I like the idea of parametrizing the test using a scale factor a lot. Probably a good habit to get into for these tests aimed at geometry/mesh. FWIW, I also tested this with scale factors below 1 and it was also successful in matching the tallies 💯

I applied these changes to the test problem presented in #2369 and the jumps in the flux at the geometric boundaries still appear. So there do seem to be some additional subtleties for cases where geometric boundaries are coincident with interior radial mesh boundaries, but I can address those in a subsequent PR that targets that particular pathology.

[pshriwise]

One tiny request to scale the problem geometry down as well as up. I tried out factors below 0.1 locally and they added a little too much time to the test -- though I'm not entirely sure why that is to be honest.

[paulromano]

@pshriwise regarding factors below 0.1, I think it's just physics -- if you make the problem really small with reflective boundaries, particles will just keep bouncing around for a long time before reaching a collision (hence why it takes a lot longer)

[pshriwise]

@pshriwise regarding factors below 0.1, I think it's just physics -- if you make the problem really small with reflective boundaries, particles will just keep bouncing around for a long time before reaching a collision (hence why it takes a lot longer)

Ah, of course. Missed the refl BC's here somehow. Thanks for incorporating that change!