Add classes for coordinate spaces and transforms #218

jp-dark · 2024-09-12T21:45:57Z

New classes:

Axis: simple data class storing a name, optional units, and optional scale
CoordinateSpace: used to define coordinate spaces for spatial data
CoordinateTransform: abstract base class for coordinate transforms
AffineTransform: implementation for a generic affine coordinate transform
ScaleTransform: implementation for a coordinate transform that scales axes (may be isotropic/uniform)
IdentityTransform: implementation for an identity transform that only applies a name change to axes

johnkerl

LGTM with minor suggestions

python-spec/src/somacore/coordinates.py

ivirshup

Looks mostly good, but I left some comments.

One important thing: "dunder methods" tend not to show up in rendered docs so I think the class docs need to explicitly say that these things are composable and @ is the operator for doing that. Alternatively I would add a compose method so that it's more discoverable (via tab completion or doc method table).

python-spec/src/somacore/coordinates.py

python-spec/src/somacore/__init__.py

python-spec/src/somacore/coordinates.py

- PR feedback

python-spec/src/somacore/coordinates.py

bkmartinjr

LGTM

python-spec/src/somacore/coordinates.py

- Drop scale from units - Use `attrs` instead of `dataclass` - Remove ABC meta class (bug)

- Add docstrings to Axis - Use `attrs` for CoordinateSpace - Add helper function for validation

python-spec/src/somacore/coordinates.py

python-spec/src/somacore/types.py

Make CoordinateSpace classes call the super.__init__ from the level above.

Co-authored-by: paul fisher <paul.fisher@tiledb.com>

Add new methods that throw value errors for mismatched axes with a more meaningful error message.

thetorpedodog

The solution to the class hierarchy situation here is good; I just have a few more suggestions (sorry) mostly about the operator implementation.

python-spec/src/somacore/coordinates.py

johnkerl

🚢

jp-dark · 2024-09-20T17:38:53Z

[sc-50289]

nguyenv · 2024-09-20T22:32:52Z

python-spec/src/somacore/coordinates.py

+
+
+@attrs.define(frozen=True)
+class CoordinateSpace(


Can we also implement __eq__ for CoordinateSpace in here?

coming back from the dead to say that attrs gives you __eq__ for free

jp-dark added 3 commits September 12, 2024 17:41

Add classes for coordinate spaces and transforms

b02dd45

Add coordinate space/transform classes to __all__

ff532b6

Update docstrings, add tests, handle init edge cases better

cbaa95f

jp-dark marked this pull request as ready for review September 13, 2024 14:51

jp-dark requested review from aaronwolen, johnkerl, bkmartinjr and ivirshup September 13, 2024 14:52

Fix linting/test in test_coordinates.py

a2b3c1f

johnkerl approved these changes Sep 13, 2024

View reviewed changes

python-spec/src/somacore/coordinates.py Outdated Show resolved Hide resolved

python-spec/src/somacore/coordinates.py Outdated Show resolved Hide resolved

python-spec/src/somacore/coordinates.py Outdated Show resolved Hide resolved

jp-dark added 2 commits September 13, 2024 11:31

Respond to PR feedback (docstring clarity + operator ambiguity)

75e77d6

Add lifecycle to docstrings

36ae037

jp-dark mentioned this pull request Sep 13, 2024

Add new spatial foundational types #219

Merged

ivirshup reviewed Sep 13, 2024

View reviewed changes

jp-dark added 3 commits September 13, 2024 15:26

Remove redundant rank methods

e4ec1f6

Change RuntimeError -> ValueError

fa4918e

- PR feedback

Add UniformScaleTransform class

40cbe3a

ivirshup reviewed Sep 13, 2024

View reviewed changes

python-spec/src/somacore/coordinates.py Outdated Show resolved Hide resolved

jp-dark added 2 commits September 13, 2024 16:36

Clean-up coordinate transform matrix multiply methods

8148c1a

Adjust names and docstrings

afe2e54

jp-dark requested a review from ivirshup September 13, 2024 20:48

bkmartinjr reviewed Sep 16, 2024

View reviewed changes

python-spec/src/somacore/coordinates.py Outdated Show resolved Hide resolved

jp-dark requested review from johnkerl and bkmartinjr September 16, 2024 16:19

bkmartinjr approved these changes Sep 16, 2024

View reviewed changes

thetorpedodog reviewed Sep 16, 2024

View reviewed changes

Update coordinate class based on feedback

9c514cd

- Drop scale from units - Use `attrs` instead of `dataclass` - Remove ABC meta class (bug)

jp-dark force-pushed the dark/coordinate-space branch from a2505a9 to 9c514cd Compare September 16, 2024 20:18

jp-dark added 2 commits September 17, 2024 07:48

Run linters

0b63287

Update attrs usage based on PR feedback

99d5df2

- Add docstrings to Axis - Use `attrs` for CoordinateSpace - Add helper function for validation

Improve __matmul__, __rmatmul__ types/errors based on PR feedback

79de67f

thetorpedodog reviewed Sep 17, 2024

View reviewed changes

python-spec/src/somacore/coordinates.py Show resolved Hide resolved

python-spec/src/somacore/coordinates.py Outdated Show resolved Hide resolved

python-spec/src/somacore/types.py Outdated Show resolved Hide resolved

jp-dark and others added 3 commits September 17, 2024 10:24

Improve CoordinateSpace inheritance based on PR feedback

893b34e

Make CoordinateSpace classes call the super.__init__ from the level above.

Fix typo python-spec/src/somacore/types.py

7350d2a

Co-authored-by: paul fisher <paul.fisher@tiledb.com>

Add helper methods for checking axes for @ based on PR feedback

7deb1e6

Add new methods that throw value errors for mismatched axes with a more meaningful error message.

jp-dark requested a review from thetorpedodog September 17, 2024 14:41

thetorpedodog reviewed Sep 17, 2024

View reviewed changes

jp-dark added 3 commits September 18, 2024 10:13

Fix error messages

d63117f

Specify sequence type for CoordinateSpace abc

509a462

Remove stray duplicate value check

64b1362

thetorpedodog reviewed Sep 18, 2024

View reviewed changes

python-spec/src/somacore/coordinates.py Show resolved Hide resolved

python-spec/src/somacore/coordinates.py Outdated Show resolved Hide resolved

jp-dark added 4 commits September 18, 2024 12:37

Add matmul coordinate transform tests and fix matmul bug

7365f37

For CoordinateTransform drop __rmatmul__ and depend only on __matmul__

6bcec59

Narrow type for inverse transforms

b896712

Fix 3.8 type error

ff235d0

johnkerl approved these changes Sep 18, 2024

View reviewed changes

aaronwolen approved these changes Sep 18, 2024

View reviewed changes

jp-dark merged commit 9cb6f80 into main Sep 18, 2024
7 checks passed

jp-dark deleted the dark/coordinate-space branch September 18, 2024 20:50

nguyenv reviewed Sep 20, 2024

View reviewed changes

ryan-williams mentioned this pull request Sep 23, 2024

Comment nits #226

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add classes for coordinate spaces and transforms #218

Add classes for coordinate spaces and transforms #218

jp-dark commented Sep 12, 2024

johnkerl left a comment

ivirshup left a comment

bkmartinjr left a comment

thetorpedodog left a comment

johnkerl left a comment

jp-dark commented Sep 20, 2024

nguyenv Sep 20, 2024

thetorpedodog Sep 20, 2024

Add classes for coordinate spaces and transforms #218

Add classes for coordinate spaces and transforms #218

Conversation

jp-dark commented Sep 12, 2024

johnkerl left a comment

Choose a reason for hiding this comment

ivirshup left a comment

Choose a reason for hiding this comment

bkmartinjr left a comment

Choose a reason for hiding this comment

thetorpedodog left a comment

Choose a reason for hiding this comment

johnkerl left a comment

Choose a reason for hiding this comment

jp-dark commented Sep 20, 2024

nguyenv Sep 20, 2024

Choose a reason for hiding this comment

thetorpedodog Sep 20, 2024

Choose a reason for hiding this comment