Skip to content
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

BUG: calculate default depth for off axis projections #5081

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
1 change: 1 addition & 0 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,7 @@ test = [
"pytest-mpl>=0.16.1",
"sympy!=1.10,!=1.9", # see https://github.com/sympy/sympy/issues/22241
"imageio!=2.35.0", # see https://github.com/yt-project/yt/issues/4966
"contourpy",
]

[project.scripts]
Expand Down
3 changes: 1 addition & 2 deletions yt/visualization/fixed_resolution.py
Original file line number Diff line number Diff line change
Expand Up @@ -639,7 +639,6 @@ def _generate_image_and_mask(self, item) -> None:
self.bounds[5] - self.bounds[4],
)
)
depth = dd.depth[0] if dd.depth is not None else None
Copy link
Contributor Author

Choose a reason for hiding this comment

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

removing this because the depth sanitization before dd creation takes care of this now.

buff = off_axis_projection(
dd.dd,
dd.center,
Expand All @@ -652,7 +651,7 @@ def _generate_image_and_mask(self, item) -> None:
no_ghost=dd.no_ghost,
interpolated=dd.interpolated,
north_vector=dd.north_vector,
depth=depth,
depth=dd.depth,
method=dd.method,
)
if self.data_source.moment == 2:
Expand Down
27 changes: 15 additions & 12 deletions yt/visualization/plot_window.py
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,11 @@ def get_window_parameters(axis, center, width, ds):


def get_oblique_window_parameters(
normal, center, width, ds, depth=None, get3bounds=False
normal,
center,
width,
ds,
depth=None,
):
center, display_center = ds.coordinates.sanitize_center(center, axis=None)
width = ds.coordinates.sanitize_width(normal, width, depth)
Expand All @@ -101,15 +105,7 @@ def get_oblique_window_parameters(

w = tuple(el.in_units("code_length") for el in width)
bounds = tuple(((2 * (i % 2)) - 1) * w[i // 2] / 2 for i in range(len(w) * 2))
if get3bounds and depth is None:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

if we initially calculate a depth equal to the diagonal + a bit, we don't need this extra logic here as the off-axis projection calls to this function will always have a depth.

# off-axis projection, depth not specified
# -> set 'large enough' depth using half the box diagonal + margin
d2 = ds.domain_width[0].in_units("code_length") ** 2
d2 += ds.domain_width[1].in_units("code_length") ** 2
d2 += ds.domain_width[2].in_units("code_length") ** 2
diag = np.sqrt(d2)
bounds = bounds + (-0.51 * diag, 0.51 * diag)
return (bounds, center)
return bounds, center


def get_axes_unit(width, ds):
Expand Down Expand Up @@ -2389,7 +2385,8 @@ class OffAxisProjectionPlot(ProjectionPlot, PWViewerMPL):
depth : A tuple or a float
A tuple containing the depth to project through and the string
key of the unit: (width, 'unit'). If set to a float, code units
are assumed
are assumed. In not set, then a depth equal to the diagonal of
the domain width plus a small margin will be used.
weight_field : string
The name of the weighting field. Set to None for no weight.
max_level: int
Expand Down Expand Up @@ -2466,6 +2463,13 @@ def __init__(
"currently supported geometries:"
f" {self._supported_geometries!r}"
)

if depth is None:
# off-axis projection, depth not specified
# -> set 'large enough' depth using half the box diagonal + margin
depth = np.linalg.norm(ds.domain_width.in_units("code_length")) * 1.0001
depth = ds.coordinates.sanitize_depth(depth)[0]

# center_rot normalizes the center to (0,0),
# units match bounds
# for SPH data, we want to input the original center
Expand All @@ -2479,7 +2483,6 @@ def __init__(
width,
ds,
depth=depth,
get3bounds=True,
)
# will probably fail if you try to project an SPH and non-SPH
# field in a single call
Expand Down
80 changes: 79 additions & 1 deletion yt/visualization/tests/test_offaxisprojection_pytestonly.py
Original file line number Diff line number Diff line change
@@ -1,14 +1,18 @@
import numpy as np
import pytest
import unyt
from numpy.testing import assert_allclose

from yt.testing import (
assert_rel_equal,
cubicspline_python,
fake_amr_ds,
fake_sph_flexible_grid_ds,
fake_sph_grid_ds,
integrate_kernel,
requires_module_pytest,
)
from yt.visualization.api import ProjectionPlot
from yt.visualization.api import OffAxisProjectionPlot, ProjectionPlot


@pytest.mark.parametrize("weighted", [True, False])
Expand Down Expand Up @@ -165,3 +169,77 @@ def makemasses(i, j, k):
# print("expected:\n", expected_out)
# print("recovered:\n", img.v)
assert_rel_equal(expected_out, img.v, 4)


@pytest.fixture
def fame_amr_ds_for_proj():
return fake_amr_ds()
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain the benefit of using a fixture here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh! I actually think I was planning to write these tests differently where a fixture would have been more useful and then forgot to remove it. I guess as-is it might save a negligible amount of time on the dataset instantiation? but I think it's clearer if I just move it back into the test itself, thanks for catching it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed!

Copy link
Member

Choose a reason for hiding this comment

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

it. I guess as-is it might save a negligible amount of time on the dataset instantiation

I don't think it would. Rather, it moves the instantiation to collection time, but overall it saves nothing. Anyway, thanks for removing it !

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but wouldn't it instantiate once (at collection time) and then re-use the ds rather than instantiating one for every set of test parameters? Or is that not how fixtures work? Mainly asking to make sure I understand it for other cases where it might actually matter.



_diag_dist = np.sqrt(3.0) # diagonal distance of a cube with length 1.
# each case is depth, center, expected integrated distance
_cases_to_test = [
(_diag_dist / 3.0, "domain_left_edge", _diag_dist / 3.0 / 2.0),
(_diag_dist * 2.0, "domain_left_edge", _diag_dist),
(_diag_dist * 4.0, "domain_left_edge", _diag_dist),
(None, "domain_center", _diag_dist),
]


@pytest.mark.parametrize("depth,proj_center,expected", _cases_to_test)
def test_offaxisprojection_depth(depth, proj_center, expected, fame_amr_ds_for_proj):
# this checks that the depth keyword argument works as expected.
# in all cases, it integrates the (index, ones) field for a normal
# pointing to the right edge corner of the domain.
#
# For the tests where the projection is centered on the left edge,
# the integrate distance will scale as depth / 2.0. When centered
# on the origin, it will scale with depth. The integrated distance
# should max out at the diagonal distance of the cube (when the depth
# exceeds the cube diagonal distance).
#
# Also note that the accuracy will depend on the buffer dimensions:
# using the default (800,800) results in accuracy of about 1 percent

ds = fame_amr_ds_for_proj

n = [1.0, 1.0, 1.0]
c = getattr(ds, proj_center)
field = ("index", "ones")

p = ProjectionPlot(ds, n, field, depth=depth, weight_field=None, center=c)

maxval = p.frb[field].max().d
assert_allclose(expected, maxval, atol=1e-2)


_sph_test_cases = [
([1.0, 1.0, 0.7], 27),
([1.0, 1.0, 1], 19),
([0.0, 0.0, 1], 9),
]


@requires_module_pytest("contourpy")
@pytest.mark.parametrize("normal_vec, n_particles", _sph_test_cases)
def test_offaxisprojection_sph_defaultdepth(normal_vec, n_particles):
# checks that particles are picked up as expected for a range of
# depths and normal vectors. Certain viewing angles will result
# in overlapping particles (since fake_sph_grid_ds aligns particles
# on a grid): n_particles is the expected number of circles in the
# resulting image for the given normal vector. Circle counts are
# calculated here using a contour generator.
from contourpy import contour_generator

ds = fake_sph_grid_ds()
c = ds.domain_center
diag_dist = np.linalg.norm(ds.domain_width)
field = ("gas", "mass")
p = OffAxisProjectionPlot(
ds, normal_vec, field, weight_field=None, center=c, width=diag_dist
)
p.render()

# get the number of circles in the plot
cg = contour_generator(z=p.frb[("gas", "mass")].d)
assert n_particles == len(cg.lines(1.0))
Loading