transform and models #52
Conversation
* add io test * [pre-commit.ci] pre-commit autoupdate (#13) updates: - [github.com/asottile/yesqa: v1.3.0 → v1.4.0](asottile/yesqa@v1.3.0...v1.4.0) - [github.com/PyCQA/flake8: 5.0.1 → 5.0.4](PyCQA/flake8@5.0.1...5.0.4) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
…ldata into feature/transform
see discussion here ome/ome-zarr-py#219
…ldata into feature/transform
for more information, see https://pre-commit.ci
…ldata into feature/transform
for more information, see https://pre-commit.ci
…ldata into feature/transform
for more information, see https://pre-commit.ci
…ldata into feature/transform
|
ok all models test are now there. the table model is a bit flaky but I think fine for now, expecting changes as we play around with datasets. @LucaMarconato IMHO merge whenever you want |
|
Great @giovp, good for me. Please @kevinyamauchi or @ivirshup merge anytime |
There was a problem hiding this comment.
Thanks for this massive effort @giovp and @LucaMarconato ! This looks like a great start. I propose to optimistically merge this tonight.
I have made a few comments below that I will turn into issues so we can address them as the code base stabilizes.
Thanks again ! 🚀
- There are a lot of commented out blocks of code. We should delete them.
- Many functions are missing docstrings
- The tests look fairly comprehensive, but it is difficult to tell what they are testing. I think we should both break them up into smaller tests and add comments/docstrings to explain what they are testing. Tests are an important form of documentation for contributors - especially contributors who are not part of writing the original code!
| @@ -87,12 +87,12 @@ def name(self) -> Optional[str]: | |||
| return self._name | |||
|
|
|||
| @property | |||
| def axes_names(self) -> List[str]: | |||
| return [ax.name for ax in self._axes] | |||
| def axes_names(self) -> Tuple[str, ...]: | |||
There was a problem hiding this comment.
I think it makes more sense grammatically to all these axis_names/axis_types
|
|
||
|
|
||
| @singledispatch | ||
| def get_transform(e: SpatialElement) -> BaseTransformation: |
There was a problem hiding this comment.
I think we should add a docstring here.
There was a problem hiding this comment.
Included this in issue #65
| @@ -0,0 +1,578 @@ | |||
| """This file contains models and schema for SpatialData""" | |||
There was a problem hiding this comment.
I think we should split up the models into separate files so it is easier to read/navigate.
| self.rotation = self._parse_list_into_array(rotation) | ||
|
|
||
| @classmethod | ||
| def _from_dict(cls, d: Transformation_t) -> Self: # type: ignore[valid-type] |
There was a problem hiding this comment.
Is this the correct type annotation?
There was a problem hiding this comment.
Yes but mypy complains when it see Self. Maybe someone have a look please.
There was a problem hiding this comment.
this is indeed weird but I think Self might be valid in newer python versions?
|
LGTM! |
This PR implements transforms (#22 and model validation and parsing (#40) for SpatialData.