Incorrect behavior for `inplace` parameter of `Surface.rotate` for some surface types.

[pshriwise]

Bug Description

The inplace option for Surface.rotate doesn't seem to be working correctly for some surfaces -- axis-aligned cones and cylinders are confirmed to be problematic for now. This is a screenshot of a surface rotation:

Steps to Reproduce

Further evidence that the returned surface is in fact a different object:

Environment

Jupyter (v0.14.0)

[paulromano]

In general, you can't rotate an axis-aligned cylinder/cone and have its class remain the same, which is why you are seeing this behavior. Perhaps the best thing to do would be to have a warning if the user tries to rotate such a surface in-place.

[eepeterson]

mmmmm this is probably my fault sorry. Although I think the inplace only means without changing the ID of that surface since it doesn't make sense to return the same object when if you rotate a ZCone it could be no longer a ZCone.

[MicahGale]

That behavior is pretty counterintuitive as inplace makes the user think they don't need to collect the results of the run. I think in this case an exception should be raised, since type mutation in place seems like a terrible idea probably (though you can technically make it work).

[pshriwise]

In general, you can't rotate an axis-aligned cylinder/cone and have its class remain the same, which is why you are seeing this behavior. Perhaps the best thing to do would be to have a warning if the user tries to rotate such a surface in-place.

mmmmm this is probably my fault sorry. Although I think the inplace only means without changing the ID of that surface since it doesn't make sense to return the same object when if you rotate a ZCone it could be no longer a ZCone.

That... makes a ton of sense! Duh. I wonder about the value of providing that option at all if we can really only guarantee it some of the time.

[eepeterson]

I think the original intent was to avoid the situation like the following cone = ZCone(surface_id=5).rotate((0, 90, 0) giving you a cone back with a different surface ID. I'm not condoning the use of manually specified IDs, but it is allowable, supported, and useful for model comparison between codes on rare occasion. In general I'm in favor of simplifying APIs and removing truly unnecessary options. The documentation could at the very least be improved to indicate that the inplace kwarg preserves the surface ID, but not necessarily the object itself. The operative property of the surfaces in the model building process is their ID and in the view of surfaces as simply mapping IDs to a series of parameters, the inplace kwarg is just modifying those parameters without creating a new entry in the map. I don't know if this warrants removing it from the code, but I'm open to arguments.

[pshriwise]

I'm not sure it warrants removal either, but, given the purpose, the term inplace doesn't quite hit the mark to me. It seems to imply that you can call this on a surface object without capturing the return value. A warning would probably be helpful as well as an update to the documentation. The point on model comparison and ID consistency is solid -- I don't think we should remove support for this option.

Functionally, my concern is that the implication of inplace is that you don't need to capture the result of the Surface.rotate method, but that is only sometimes true. I think it would be more clear to rename inplace to preserve_id (or something along those lines) and always return a new surface object to remove any ambiguity from the situation. It seems this would accomplish the goal of supporting manually specified IDs for benchmarking/model conversion or what have you, but provide a more clear definition of the method.

I'm not saying we should rush to execute this by any means -- not our biggest fire in OpenMC by far 😄 But I thought it'd be good to note for future API iteration.