[wk-libs] Allow neg bbox topleft (#589)

* upgrade pdoc to fix some linkify behavior
* warn when layers are not uploaded due to linking
* allow negative bbox.topleft, always convert size to positive pendant
* add explaining comment
* add changelog entry
* don't cancel ci builds on master
* ci syntax fix

Author: jstriebel

.github/workflows/ci.yml

@@ -4,7 +4,7 @@ on: push
 
 concurrency:
   group: ${{ github.workflow }}-${{ github.ref }}
-  cancel-in-progress: true
+  cancel-in-progress: ${{ github.ref != 'refs/heads/master' }}
 
 jobs:
   changes:

docs/poetry.lock

@@ -13,7 +13,7 @@ LinkChecker = "^10.0.1"
 mkdocs = "1.2.2"
 mkdocs-material = "^7.3.0"
 mkdocs-redirects = "^1.0.3"
-pdoc = "^9.0.1"
+pdoc = { git = "https://github.com/mitmproxy/pdoc.git", rev = "1574222ab0568e072dc04e3569027d39aa124256"}
 webknossos = { path = "../webknossos/", develop = true }
 wkcuber = { path = "../wkcuber/", develop = true }
 

docs/pyproject.toml

@@ -15,6 +15,8 @@ For upgrade instructions, please check the respective *Breaking Changes* section
  - Added AnnotationInfo, Project and Task classes for handling annotation information and annotation project administration. [#574](https://github.com/scalableminds/webknossos-libs/pull/574):
 
 ### Changed
+- Lifted the restriction that `BoundingBox` cannot have a negative topleft (introduced in v0.9.0). Also, negative size dimensions are flipped, so that the topleft <= bottomright,
+  e.g. `BoundingBox((10, 10, 10), (-5, 5, 5))` -> `BoundingBox((5, 10, 10), (5, 5, 5))`. [#589](https://github.com/scalableminds/webknossos-libs/pull/589)
 
 ### Fixed
 
@@ -65,6 +67,7 @@ For upgrade instructions, please check the respective *Breaking Changes* section
       the default behaviour changes from starting at absolute (0, 0, 0) to the layer's bounding box
     * `(Mag)View.get_view`: read_only is a keyword-only argument now
     * `MagView.get_bounding_boxes_on_disk()` now returns an iterator yielding bounding boxes in Mag(1)
+    * `BoundingBox` cannot have negative topleft or size entries anymore (lifted in v0.9.4).
   - **Deprecations**
     The following usages are marked as deprecated with warnings and will be removed in future releases:
     * Using the `offset` parameter for `read`/`write`/`get_view` in MagView and View is deprecated.

webknossos/Changelog.md

@@ -92,3 +92,20 @@ def test_align_with_mag_against_numpy_implementation(
         # The slower numpy implementation is wrong for very large numbers:
         if all(i < 12e15 for i in bb.bottomright):
             assert bb.align_with_mag(mag, ceil) == slow_np_result
+
+
+def test_negative_size() -> None:
+    assert BoundingBox((10, 10, 10), (-5, 5, 5)) == BoundingBox((5, 10, 10), (5, 5, 5))
+    assert BoundingBox((10, 10, 10), (-5, 5, -5)) == BoundingBox((5, 10, 5), (5, 5, 5))
+    assert BoundingBox((10, 10, 10), (-5, 5, -50)) == BoundingBox(
+        (5, 10, -40), (5, 5, 50)
+    )
+
+
+@given(bbox=infer)
+def test_negative_inversion(
+    bbox: BoundingBox,
+) -> None:
+    """Flipping the topleft and bottomright (by padding both with the negative size)
+    results in the original bbox, as negative sizes are converted to positive ones."""
+    assert bbox == bbox.padded_with_margins(-bbox.size, -bbox.size)

webknossos/tests/test_bounding_box.py

@@ -1,4 +1,5 @@
 import os
+import warnings
 from functools import lru_cache
 from pathlib import Path
 from tempfile import TemporaryDirectory
@@ -85,6 +86,10 @@ def upload_dataset(
     context = _get_context()
     layer_names_to_link = set(i.new_layer_name or i.layer_name for i in layers_to_link)
     if len(layer_names_to_link.intersection(dataset.layers.keys())) > 0:
+        warnings.warn(
+            "Excluding the following layers from upload, since they will be linked: "
+            + f"{layer_names_to_link.intersection(dataset.layers.keys())}"
+        )
         with TemporaryDirectory() as tmpdir:
             tmp_ds = dataset.shallow_copy_dataset(
                 tmpdir, name=dataset.name, layers_to_ignore=layer_names_to_link

webknossos/webknossos/client/_upload_dataset.py

@@ -499,6 +499,9 @@ def bounding_box(self, bbox: BoundingBox) -> None:
         """
         Updates the offset and size of the bounding box of this layer in the properties.
         """
+        assert (
+            bbox.topleft.is_positive()
+        ), f"Updating the bounding box of layer {self} to {bbox} failed, topleft must not contain negative dimensions."
         self._properties.bounding_box = bbox
         self.dataset._export_as_json()
 

webknossos/webknossos/dataset/layer.py

@@ -371,6 +371,9 @@ def read(
             f"The size ({mag1_bbox.size} in mag1) contains a zero. "
             + "All dimensions must be strictly larger than '0'."
         )
+        assert (
+            mag1_bbox.topleft.is_positive()
+        ), f"The offset ({mag1_bbox.topleft} in mag1) must not contain negative dimensions."
 
         return self._read_without_checks(mag1_bbox.in_mag(self._mag))
 
@@ -525,6 +528,9 @@ def get_view(
                 f"The size ({mag1_bbox.size} in mag1) contains a zero. "
                 + "All dimensions must be strictly larger than '0'."
             )
+        assert (
+            mag1_bbox.topleft.is_positive()
+        ), f"The offset ({mag1_bbox.topleft} in mag1) must not contain negative dimensions."
 
         if not read_only:
             assert self.bounding_box.contains_bbox(mag1_bbox), (

webknossos/webknossos/dataset/view.py

@@ -1,7 +1,7 @@
 import json
 import re
 from collections import defaultdict
-from typing import Any, Dict, Generator, Iterable, List, Optional, Tuple, Union, cast
+from typing import Dict, Generator, Iterable, List, Optional, Tuple, Union, cast
 
 import attr
 import numpy as np
@@ -16,15 +16,16 @@ class BoundingBox:
     size: Vec3Int = attr.field(converter=Vec3Int)
     bottomright = attr.field(init=False)
 
-    @topleft.validator
-    def validate_topleft(self, _attribute: Any, value: Vec3Int) -> None:
-        assert value.is_positive(), "topleft of a BoundingBox must not be negative"
-
-    @topleft.validator
-    def validate_size(self, _attribute: Any, value: Vec3Int) -> None:
-        assert value.is_positive(), "size of a BoundingBox must not be negative"
-
     def __attrs_post_init__(self) -> None:
+        if not self.size.is_positive():
+            # Flip the size in negative dimensions, so that the topleft is smaller than bottomright.
+            # E.g. BoundingBox((10, 10, 10), (-5, 5, 5)) -> BoundingBox((5, 10, 10), (5, 5, 5)).
+            negative_size = self.size.pairmin(Vec3Int.zeros())
+            new_topleft = self.topleft + negative_size
+            new_size = self.size.pairmax(-self.size)
+            object.__setattr__(self, "topleft", new_topleft)
+            object.__setattr__(self, "size", new_size)
+
         # Compute bottomright to avoid that it's recomputed every time
         # it is needed.
         object.__setattr__(self, "bottomright", self.topleft + self.size)
@@ -238,9 +239,9 @@ def intersected_with(
     ) -> "BoundingBox":
         """If dont_assert is set to False, this method may return empty bounding boxes (size == (0, 0, 0))"""
 
-        topleft = np.maximum(self.topleft.to_np(), other.topleft.to_np())
-        bottomright = np.minimum(self.bottomright.to_np(), other.bottomright.to_np())
-        size = np.maximum(bottomright - topleft, (0, 0, 0))
+        topleft = self.topleft.pairmax(other.topleft)
+        bottomright = self.bottomright.pairmin(other.bottomright)
+        size = (bottomright - topleft).pairmax(Vec3Int.zeros())
 
         intersection = BoundingBox(topleft, size)
 
@@ -257,8 +258,8 @@ def extended_by(self, other: "BoundingBox") -> "BoundingBox":
         if other.is_empty():
             return self
 
-        topleft = np.minimum(self.topleft, other.topleft)
-        bottomright = np.maximum(self.bottomright, other.bottomright)
+        topleft = self.topleft.pairmin(other.topleft)
+        bottomright = self.bottomright.pairmax(other.bottomright)
         size = bottomright - topleft
 
         return BoundingBox(topleft, size)