ome / ome-model

OME model (specification, code generator, implementation)
Other
13 stars 26 forks source link

ome_model: channel fixes #120

Closed sbesson closed 4 years ago

sbesson commented 4 years ago

This PR inclues a set of improvements for the Image/Pixels/Channel created by the ome_model library driven by in-progress IDR work on HCS RGB companion files /cc @dominikl

Changes

Testing

Without this PR the following examples should fail during the channel validation step. With this PR, it should create a valid OME-XML:

from ome_model.experimental import Image, create_companion
image = Image("test", 512, 512, 1, 3, 1)
image.add_tiff("test.tiff")
create_companion(images=[image])
from ome_model.experimental import Image, create_companion
image = Image("test", 512, 512, 1, 3, 1)
image.add_channel(samplesPerPixel=3)
image.add_tiff("test.tiff")
create_companion(images=[image])

In addition to manual testing, commits f22547e and following add Python unit tests covering the constructors of the Image and Channel classes as well as a tox infrastructure used byTravis to run these Python tests in a separate job.

Target: 6.2.0

dominikl commented 4 years ago

Looks good, but with your second testing example I'll get:

AssertionError: {'Image': {'ID': 'Image:0', 'Name': 'test'}, 'Pixels': {'ID': 'Pixels:0:0', 'DimensionOrder': 'XYZTC', 'Type': 'uint16', 'SizeX': '512', 'SizeY': '512', 'SizeZ': '1', 'SizeT': '1', 'SizeC': '3'}, 'Channels': [<ome_model.experimental.Channel object at 0x7fe4463cd400>], 'TIFFs': [<ome_model.experimental.TiffData object at 0x7fe44559ab38>]}

And I can't find where/if the tests are running in the travis output, maybe I'm just missing what to look for?

sbesson commented 4 years ago

Edited the snippet in the PR description, the default dimension order is XYZTC and my example had an image with SizeC=1 and one channel with SamplesPerPixels=3. So the AssertionError was expected (although the error message could be improved).

The PR changes had a third job to the Travis build where the Python library is installed and tests are executed - see https://travis-ci.org/github/ome/ome-model/jobs/718428821. Locally, this can be tested by running tox -e py36 or tox -e py37 depending on your environment.

dominikl commented 4 years ago

I think it was correct, because Image("test", 512, 512, 1, 1, 3) now gets interpreted as t=3, doesn't it?

sbesson commented 4 years ago

Yes, tou are correct. The problem was that my channel samples comparison was too stringent (< while it should have been <=) and my test was not checking the case of 1 RGB channel only. I have adjusted the test accordingly in https://github.com/ome/ome-model/pull/120/commits/2905ea9a01fed4f3dd47207822b183e304c27fdb so that it failed as expected by your testing and fixed the validation in https://github.com/ome/ome-model/pull/120/commits/49d212302bd30541e55ef64409b14377b069cd3a.

joshmoore commented 4 years ago

Happy with a general code review. Also happy to open issue for "add ome_model version to output". I do wonder (similar to my recent ponderings around ome-zarr-py) if we don't want to hook this repository up to a MxN matrix of file producers/consumers, something like:

| V producer \ consumer >   | bio-formats                       | omero             | ome-zarr-py                       | omero-ms-zarr     |
|-------------------------  |---------------------------------  |----------------   |--------------------------------   |---------------    |
| ome-model-py (TIFF)       | Easy                              | Easy              | with bf2raw (needs ZarrWriter)    | N/A (yet)         |
| bio-formats (TIFF)        | Easy                              | Easy              | "                                 | "                 |
| omero (TIFF Export)       | Easy                              | Easy              |                                   | "                 |
| omero-cli-zarr (ZARR)     | with raw2ometiff and ZarrReader   | as bio-formats    | Easy                              | "                 |
| omero-ms-zarr (ZARR)      | "                                 | "                 | Easy                              | "                 |
|                           |                                   |                   |                                   |                   |
sbesson commented 4 years ago

Thanks. Merging and I'll open 2 follow-up PRs: one for plane metadata and one for the version output.

Regarding compatibility, the closest official representation is the paragraph about tooling in https://docs.openmicroscopy.org/ome-model/6.1.2/. I could certainly see this turning into a producer/consumer or maybe read/write support matrix that we would keep updating as the community tooling grows.

I would assume we would need to discuss representation as I could imagine one table per container format ie. TIFF/ZARR.