MeshFilter rotation - solution to issue #3166

[zoeprieto]

Description

This is a solution for issue #3166. It incorporates the possibility to rotate a MeshFilter. The approach was based on what is implemented today to adjust particle positions for rotated fills. It should be noted that in both cases the translation is always performed first and then the rotation.

Fixes #3166

Checklist

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

[rherrero-pf]

+1 I think this should be merged since it will be a good feature

[nickschw2]

+2 this would be fantastic to have, thanks for the work @zoeprieto

[nickschw2]

Is the only thing preventing this PR from pushing to main the updates to documentation and a review from @paulromano ? I could update the documentation in a way that parallels MeshFilter translation. It would be great to have this feature in the next release!

[pshriwise]

@nickschw2 I'd be happy to review and merge, I've just been tied up with other things. I'll push it up my list and get to it soon! Glad to hear it will be of use!

[nickschw2]

Thanks @pshriwise! I don't have permission to push to this branch, but the only change in documentation I caught is that x, y, and z axes that the filled universe should be rotated should be changed to x, y, and z axes that the mesh should be rotated.

[ebknudsen]

This would be great to have in. I have PR on hexagonal mesh that could benefit from this as well (I'd have to check if I need to change anything of course🙂)

[pshriwise]

Just got done with a more thorough review here. When trying this out I noticed that the 3-float version of a rotation for angles around the principle axes wasn't round-tripping to and from XML so I added a fix and test for that.

Then I got a little suspicious about rotate vs. inverse_rotate. I changed the angle in the new regression test to (0, 0, 10) and noticed that the rotation was clockwise about the z-axis, which goes against the convention of counterclockwise. When transforming positions/directions for tallies, we want to apply the inverse rotation as we are transforming this information onto the unmodified version of the mesh. The translation is applied as an inverse for the same reason. Thank you for that test @zoeprieto! It made it easy to try this new feature out.

We do this to simplify the process of finding bins as we can simply treat the mesh tallying as normal once the track information has been adjusted.

Here's a couple of images before and after the changes I've just pushed here:

Before

After

I've left the test set to a 10 degree rotation -- a rotation of 180 degrees produces the same results regardless of which direction the mesh is rotated.

[rherrero-pf]

@pshriwise do we see any blockers for adding this to the next release? I would find this feature really helpful.

[pshriwise]

Yes absolutely @rherrero-pf. Apologies to you and @zoeprieto for making this wait so long.

[shimwell]

This PR looks super close and it looks like CI was passing on this PR but stopped passing after the "Merge branch 'develop' into MeshRotation commit. It appears that the regression test openmc/test/regression_tests/filter_rotations/test.py is now failing, does the results_true.data need updating

[pshriwise]

Thanks for latest commit @zoeprieto! I noted that they needed an update last night, but being less familiar with this test I wanted to check it with you. Are the new results in line w/ your expectations?

[zoeprieto]

No problem! I ran the tests on my computer and they work fine, but the results I get are not exactly the same as in the GitHub test. Therefore, I copied the results I got in the GitHub test into the results_true.dat file. I understand that this is correct in this case. As for everything else, it's fine with me.

[pshriwise]

Thanks for holding off on this @shimwell! I was staring at this again and realized that my change above using Position::inverse_rotate instead of Position::rotate is incorrect!

I proved this to myself by applying the same rotation to the geometry in which case the geometry boundaries should again align with the mesh element boundaries but I saw a rotation twice the angle instead! Reverting https://github.com/openmc-dev/openmc/pull/3176/commits/6c929563f4594a967d88f820537cf21ec4219973 fixed this issue:

On the left is the rotated mesh results for an unrotated geometry. On the right is the result for a rotated mesh and geometry -- the geometry boundaries align as expected in this case.

The code in MeshFilter::set_rotation populates a rotation matrix using the negative of the input angles, resulting in an inverse rotation matrix. So applying rotate to transform particle positions/directions onto the unmodified mesh object is actually correct. This is of course also true for the Cell::rotation_ that this feature is based on.

This doesn't have anything to do with the changes in this PR, but I'm not a huge fan of this current state b/c the attribute set is named rotation_ not inverse_rotation and when functions like openmc_cell_get_rotation are called this means that the inverse matrix is returned. To add to the confusion, the original angles are provided at the end of the rotation matrix as well. The naming of the attribute could arguably be named based on what frame your rotating to/from but I think the fact that we return a rotation matrix that's the inverse of the angles at the end of the data array should be addressed.

At any rate, I've updated the test result again. Given the back and forth on this, if someone else could confirm my thinking I'd appreciate it before we merge.

[pshriwise]

@zoeprieto and I have conferred on this PR and agree that it's good to go.