-
Notifications
You must be signed in to change notification settings - Fork 280
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ENH: cartesian cutting planes through non-cartesian geometries #4847
base: main
Are you sure you want to change the base?
Conversation
mainly a WIP because I found a small bug with an edge case while writing up examples, but feel free to jump in. |
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
@@ -72,12 +72,16 @@ def __init__(self, ds, field_parameters, data_source=None): | |||
self.field_parameters.update(data_source.field_parameters) | |||
self.quantities = DerivedQuantityCollection(self) | |||
|
|||
def _get_selector_class(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note to reviewers: this change was needed so that the selector that is chosen can depend on the geometry. this method gets over-ridden in the new selector object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't remember if this was something I experimented with or not, but it seems like a good idea. Although, I wonder if we could just wrap it in the selector
property itself?
one final note to self so i don't forget: the edge case currently failing is in bbox intersection with a spherical shell and the plane falling on one side of all the edges of a large spherical element (kinda like an extreme version of the cusp problem). |
I love it ! One quick comment now is that I'm not sure about the name: I don't get it at first, and when I do I find myself expecting more than it really is. In other words, "mixed coordinate cutting planes" is confusing to me. Would "cartesian cutting planes" be an accurate description ? |
Yup, I'm not crazy about the name either actually, it's just where I ended up when this got to the point where it was ready to initially share :) "cartesian cutting planes" is accurate. Maybe Also, I did go back and forth on the best way to expose the new functionality given that |
Something like |
renamed to |
pre-commit.ci autofix |
@mathewturk asked in passing whether the intersections match what you'd get from calculating plane-sphere intersections... and then I went on a deep dive over in this notebook to check. The notebooks calculates the area of intersection for the cartesian cutting plane from a fixed res buff of the so as long as you're using a frb with resolution above ~100 in each dimension it'll do a decent job. The notebook has some other tests in it to verify things are working as expected (varying the plane position, varying the dataset bounds). |
oh and the last push adds an image test, so that test suite will fail... i'll open a PR to the image answer repo later on. |
feeling pretty good about where this is at! so just switched out of draft mode. |
Requesting a review from myself so I don't forget about it, but I can't make any promises as to when I'll be able to actually review this massive PR. Hopefully sometimes this month. |
This PR introduces a "Cartesian Cutting Plane", which allows you to define a cutting plane in cartesian coordinates and extract a fixed resolution buffer from the intersection with a non-cartesian 3D dataset. This PR only enables functionality for data in spherical coordinates, but it should be straightforward to apply to other 3D non-cartesian coordinates. Visualization is limited to constructing fixed resolution buffers at present.
Examples
See the included new example notebook for more examples, here is a direct link to the rendered file: link.
But here's a couple:
note that the curvature in the z values in the left plot arise because we're plotting the z value of cell centers.
One of my favorites applications is that this PR allows you to easily create cross sections!
See the notebook for some more cases.
Some notes
I've gone through several iterations of this the past couple months, many dead ends and many commits (most of which I squashed before PR creation). So here are some extra notes with comments on possible alternate approaches:
design choices
YTCuttingPlaneMixedCoords
YTCartesianCuttingPlane
) is accessed fromds.cutting_mixed
ds.carteisan_cutting
. @matthewturk and I chatted a bit about whether there's much utility in the currentds.cutting
for non-cartesian geometries -- it may make more sense to have this PR simply change howds.cutting
works with non-cartesian geometries rather than introduce a new data object. But I thought initial review would be easier with a separate selection object. If others agree I can refactor to removeds.cartesian_cutting
and tie in the new behavior tods.cutting
.yt.utilities.lib.coordinate_utilities.cartesian_bboxes()
that is not explicitly used in the rest of the PR (it calls functions that are used by the rest of the PR). The main reason is so that I can use it over inyt_idv
for pre-computing bounding boxes in our spherical-volume rendering attempts over there.limitations and future PRs
PlotWindow
interface due to all the validations for axis names and coordinate centers, so at present theYTCartesianCuttingPlane
only implements ato_frb
and not ato_pw
, so some manual plotting is required. There's enough to review here (and IMO enough useful functionality) that I'd rather leave anyPlotWindow
integration to a future PR...Related PRs
This PR probably replaces #4752 (the present PR actually borrowed one early commit from there, which I've kept in the commit history here. I ended up pretty much re-writing the code changes related to that commit, so while I could scrub it from the commit history here, I like having it as it reflects the development history more accurately. but if it's confusing, I can rebase here).
This PR partially addresses #4750 but until the work here fully integrates with yt's plotting interface I think that PR should stay open.