Skip to content

Removing YTSphereTooSmall #5211

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Aug 28, 2025
Merged

Removing YTSphereTooSmall #5211

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1102039
Removing this YTSphereTooSmall
cphyc Jul 17, 2025
458ab76
Manually trigger creation of index
cphyc Jul 31, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 2 additions & 9 deletions yt/data_objects/selection_objects/spheroids.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
validate_sequence,
)
from yt.units import YTArray
from yt.utilities.exceptions import YTEllipsoidOrdering, YTException, YTSphereTooSmall
from yt.utilities.exceptions import YTEllipsoidOrdering, YTException
from yt.utilities.logger import ytLogger as mylog
from yt.utilities.math_utils import get_rotation_matrix
from yt.utilities.on_demand_imports import _miniball
Expand Down Expand Up @@ -57,12 +57,6 @@ def __init__(
super().__init__(center, ds, field_parameters, data_source)
# Unpack the radius, if necessary
radius = fix_length(radius, self.ds)
if radius < self.index.get_smallest_dx():
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't this where we should be adding a self.ds.index for backward compat ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so. We do not want the index to be computed unless strictly necessary (this is a costly operation). If we add self.ds.index here, anytime you build a sphere, you'll have an index created along the way, which you don't with ds.region for example.

Note that I agree this is the current behaviour, but removing it is one of the motivation behind this PR :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @cphyc. Taking this out makes these objects significantly lighter.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course, but the fact that we needed to modify tests indicates there's a very high chance it also breaks user code, doesn't it ?
I agree we should aim at delaying this expensive operation, but I don't think this PR is the correct time to do it.

Copy link
Member Author

@cphyc cphyc Jul 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me, the issue is that we have some public APIs that require the index to be built — ds.field_info being one of them — yet they don't all trigger the creation of the index.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. This is the root problem. I still don't think we should knowingly break existing users before we form a better plan

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these tests are not reflecting real usage. One would never really access the field info container in this way. That said, the brittleness of field_info needing other actions before it can be access has always bugged me. PR #5250 is a potential fix for this that would allow us to remove the ds.index calls in these tests.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One would never really access the field info container in this way.

end users certainly wouldn't, but can we say this with confidence about downstream apps and libraries ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No guarantees, but it's hard to imagine. That this previously worked was a side-effect of suboptimal functionality. This is a potentially huge performance improvement. I'm in favor of merging this whether or not it breaks things downstream.

raise YTSphereTooSmall(
ds,
radius.in_units("code_length"),
self.index.get_smallest_dx().in_units("code_length"),
)
self.set_field_parameter("radius", radius)
self.set_field_parameter("center", self.center)
self.radius = radius
Expand Down Expand Up @@ -187,8 +181,7 @@ def __init__(
self._A = self.ds.quan(A, "code_length")
self._B = self.ds.quan(B, "code_length")
self._C = self.ds.quan(C, "code_length")
if self._C < self.index.get_smallest_dx():
raise YTSphereTooSmall(self.ds, self._C, self.index.get_smallest_dx())

self._e0 = e0 = e0 / (e0**2.0).sum() ** 0.5
self._tilt = tilt

Expand Down
10 changes: 0 additions & 10 deletions yt/utilities/exceptions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,16 +62,6 @@ def __str__(self):
return msg


class YTSphereTooSmall(YTException):
def __init__(self, ds, radius, smallest_cell):
self.ds = ds
self.radius = radius
self.smallest_cell = smallest_cell

def __str__(self):
return f"{self.radius:0.5e} < {self.smallest_cell:0.5e}"


class YTAxesNotOrthogonalError(YTException):
def __init__(self, axes):
self.axes = axes
Expand Down
3 changes: 3 additions & 0 deletions yt/visualization/volume_rendering/tests/test_composite.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,9 @@ def tearDown(self):
def test_composite_vr(self):
ds = fake_random_ds(64)
dd = ds.sphere(ds.domain_center, 0.45 * ds.domain_width[0])

# Trigger creation of index
ds.index
ds.field_info[ds.field_list[0]].take_log = False

sc = Scene()
Expand Down
3 changes: 3 additions & 0 deletions yt/visualization/volume_rendering/tests/test_points.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,9 @@ def tearDown(self):
def test_points_vr(self):
ds = fake_random_ds(64)
dd = ds.sphere(ds.domain_center, 0.45 * ds.domain_width[0])

# Trigger creation of index
ds.index
ds.field_info[ds.field_list[0]].take_log = False

sc = Scene()
Expand Down
2 changes: 2 additions & 0 deletions yt/visualization/volume_rendering/tests/test_zbuff.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,8 @@ def tearDown(self):
def test_composite_vr(self):
ds = fake_random_ds(64)
dd = ds.sphere(ds.domain_center, 0.45 * ds.domain_width[0])
# Trigger creation of index
ds.index
ds.field_info[ds.field_list[0]].take_log = False

sc = Scene()
Expand Down
Loading