-
Notifications
You must be signed in to change notification settings - Fork 299
Include bbox() functions for YTCoveringGrids and YTIntersectionContainer3D #5328
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
base: main
Are you sure you want to change the base?
Include bbox() functions for YTCoveringGrids and YTIntersectionContainer3D #5328
Conversation
chrishavlin
left a comment
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.
ok, took me a while to realize that this is actually a bug not a new feature... your description clearly said it but I read too fast. For others, here's a MRE on main:
MRE
import yt
ds = yt.load_sample("IsolatedGalaxy")
sp = ds.sphere(ds.domain_center, 0.2)
cg_intx = ds.covering_grid(2, left_edge=[.45, .45, .45], dims=[32, 32, 32], data_source=sp)
cg_intx.get_bbox() # (unyt_array([0., 0., 0.], 'code_length'), unyt_array([1., 1., 1.], 'code_length'))
# should be
ds.covering_grid(2, left_edge=[.45, .45, .45], dims=[32, 32, 32]).get_bbox()
# (unyt_array([0.45, 0.45, 0.45], 'code_length'), unyt_array([0.7, 0.7, 0.7], 'code_length'))But I don't think the logic of the fix is quite right: when you provide a data source to the covering grid, the bounding box of the covering grid does not change. The intersection is only used to fill the values. e.g., consider
import yt
import matplotlib.pyplot as plt
import numpy as np
ds = yt.load_sample("IsolatedGalaxy")
sp = ds.sphere(ds.domain_center, .5)
cg_intx = ds.covering_grid(2, left_edge=[.5, .5, .5], dims=[64, 64, 64], data_source=sp)
plt.imshow(np.log10(cg_intx[("gas", "density")][:,:,16]))
plt.show()You still get a regularized array that covers (0.5, 0.5, 0.5) to (1., 1., 1.):
It's just that values are not set outside the intersection.
But by the logic of your implementation, the bounding box would be that of the sphere object ((0., 0., 0.), (1, 1, 1)).
The get_bbox() is clearly broken, but the info is correctly stored in the left_edge and right_edge attrs:
cg_intx.left_edge
unyt_array([0.5, 0.5, 0.5], 'code_length')
cg_intx.right_edge
unyt_array([1., 1., 1.], 'code_length')
so IMO, the proper fix is to just return those left/right edges in get_bbox:
def get_bbox():
return (self.left_edge, self.right_edge)Also, from a dev standpoint: would be great to add a pytest unit test for this since you don't need a full dataset to test: you can use the yt.testing.fake_amr_ds (or probably any of the fake datasets).
|
Right I slightly misunderstood what the resulting bounding box should be, thanks for the clarification! |
The bounding box is already properly set in the left and right edges, so return them.
|
I've revised a bit what I actually wanted to fix with using the covering grid, which is loading the data relevant to the constructed object. So what is important is the bbox of _data_source, not the convering grid itself. Currently there is no get_bbox() function for a YTIntersectionContainer3D object so it just returns the full simulation volume. I've now added a get_bbox() function which gets the bbox for the data which we want to load. This is relevant for my use case (RAMSES dataset), as we determine which files to load based on if those files overlap with the data object. |
|
great! thanks for the update -- on quick look the fix looks good to me, but I wanna play around with it a bit (probably won't get to it until next Mon/Tues). But can you add some unit tests? Happy to point you in the right direction if you need hand figuring out yt's pytest testing framework, just let me know. |
|
I had not made a unit test before so hopefully I put it in the right place. |
yt/data_objects/tests/test_bbox.py
Outdated
| le_sp, re_sp = sp.get_bbox() | ||
|
|
||
| assert_equal(le_sp, cgds_ds_bbox[0]) | ||
| assert_equal(cgds_ds_bbox[1], cgds_ds_bbox[1]) |
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.
Is this a typo?
| assert_equal(cgds_ds_bbox[1], cgds_ds_bbox[1]) | |
| assert_equal(re_sp, cgds_ds_bbox[1]) |
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.
Yes its a typo but it should actually be comparing cgds_bbox[1] to cgds_ds_bbox[1] (the right edge of the covering grid to the right edge of the covering grid's datasource)
| cgds_bbox = cgds.get_bbox() | ||
| assert_equal(cg_bbox, cgds_bbox) | ||
|
|
||
| # The bounding box of the cg's data source should be the left edge of sp and right edge of cgds |
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 find the comment quite cryptic. Do you mean that the covering grid's datasource (sp) should have the same bbox as sp?
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.
So they way I've set up this test is I've made a covering grid in the lower left of the box, and placed a sphere in the center. Parts of the sphere is in the bounding box but some of it is not.
The covering grid which includes the sphere should have a data source which is the intersection of these two objects, and whose bounding box should be the lower left of the sphere, and the upper right of the covering grid.
|
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
PR Summary
When get the bounding box of a YTCoveringGrid object which has also been given a data_source (making an intersection object), the resulting bounding box is the full simulation. Here I add a
get_bbox()function to YTCoveringGrid which sets the bounding box as the smallest box which encloses both regions.