Repository: scipp/plopp

Issue:
Default pixel size in pythreejs plotting assumes `unit='m'`?
Using `plot(projection='3d', positions=var_with_mm_unit)` leads to very large pixel sizes. Does it assume the unit is `m` when determining a default size?

Discussion:

Patch:
diff --git a/src/plopp/graphics/point_cloud.py b/src/plopp/graphics/point_cloud.py
index c368b2e..e8406cf 100644
--- a/src/plopp/graphics/point_cloud.py
+++ b/src/plopp/graphics/point_cloud.py
@@ -3,7 +3,7 @@
 
 import numpy as np
 import scipp as sc
-from typing import Tuple
+from typing import Tuple, Union
 
 
 class PointCloud:
@@ -26,15 +26,14 @@ class PointCloud:
         The opacity of the points.
     """
 
-    def __init__(
-            self,
-            *,
-            x: str,
-            y: str,
-            z: str,
-            data: sc.DataArray,
-            pixel_size: float = 1,  # TODO: pixel_size should have units
-            opacity: float = 1):
+    def __init__(self,
+                 *,
+                 x: str,
+                 y: str,
+                 z: str,
+                 data: sc.DataArray,
+                 pixel_size: Union[sc.Variable, float, int] = 1,
+                 opacity: float = 1):
         """
         Make a point cloud using pythreejs
         """
@@ -44,7 +43,17 @@ class PointCloud:
         self._x = x
         self._y = y
         self._z = z
+
         self._pixel_size = pixel_size
+        if hasattr(self._pixel_size, 'unit'):
+            if len(set([self._data.meta[dim].unit for dim in [x, y, z]])) > 1:
+                raise ValueError(
+                    f'The supplied pixel_size has unit {self._pixel_size.unit}, but '
+                    'the spatial coordinates do not all have the same units. In this '
+                    'case the pixel_size should just be a float with no unit.')
+            else:
+                self._pixel_size = self._pixel_size.to(
+                    dtype=float, unit=self._data.meta[x].unit).value
 
         self.geometry = p3.BufferGeometry(
             attributes={


Test patch:
diff --git a/tests/graphics/point_cloud_test.py b/tests/graphics/point_cloud_test.py
index 15e5fe8..a05aeee 100644
--- a/tests/graphics/point_cloud_test.py
+++ b/tests/graphics/point_cloud_test.py
@@ -6,6 +6,7 @@ from plopp.graphics.point_cloud import PointCloud
 
 import numpy as np
 import scipp as sc
+import pytest
 
 
 def test_creation():
@@ -50,3 +51,41 @@ def test_get_limits_flat_panel():
     assert sc.identical(ylims[1], da.meta['y'].max() + sc.scalar(0.5 * pix, unit='m'))
     assert sc.identical(zlims[0], sc.scalar(-0.5 * pix, unit='m'))
     assert sc.identical(zlims[1], sc.scalar(0.5 * pix, unit='m'))
+
+
+def test_pixel_size():
+    """
+    We make a reference points cloud because additional factors are potentially added to
+    the size, depending on the device pixel ratio. Making a reference with a default
+    size of 1 makes it easier to test.
+    """
+    da = scatter_data()
+    reference = PointCloud(data=da, x='x', y='y', z='z', pixel_size=1)
+    cloud = PointCloud(data=da, x='x', y='y', z='z', pixel_size=sc.scalar(2, unit='m'))
+    assert cloud.material.size == 2.0 * reference.material.size
+
+
+def test_pixel_size_unit_conversion():
+    da = scatter_data()
+    reference = PointCloud(data=da, x='x', y='y', z='z', pixel_size=1)
+    cloud = PointCloud(data=da,
+                       x='x',
+                       y='y',
+                       z='z',
+                       pixel_size=sc.scalar(350, unit='cm'))
+    assert cloud.material.size == 3.5 * reference.material.size
+    with pytest.raises(sc.UnitError):
+        PointCloud(data=da, x='x', y='y', z='z', pixel_size=sc.scalar(350, unit='s'))
+
+
+def test_pixel_size_cannot_have_units_when_spatial_dimensions_have_different_units():
+    da = scatter_data()
+    new_x = da.coords['x'].copy()
+    new_x.unit = 's'
+    da.coords['x'] = new_x
+    reference = PointCloud(data=da, x='x', y='y', z='z', pixel_size=1)
+    with pytest.raises(ValueError, match='The supplied pixel_size has unit'):
+        PointCloud(data=da, x='x', y='y', z='z', pixel_size=sc.scalar(2.5, unit='m'))
+    # Ok if no unit supplied
+    cloud = PointCloud(data=da, x='x', y='y', z='z', pixel_size=2.5)
+    assert cloud.material.size == 2.5 * reference.material.size