mobie / mobie-utils-python

Python tools for MoBIE
MIT License
9 stars 5 forks source link

pixel size mess in OME-Zarr #135

Closed martinschorb closed 7 months ago

martinschorb commented 8 months ago

Hi @constantinpape ,

when I provide the pixel size as list(str) the downscaling workflow produces strange pixel size metadata:

.zattrs:

{"multiscales":[{"axes":[{"name":"z","type":"space","unit":"nm"},{"name":"y","type":"space","unit":"nm"},
{"name":"x","type":"space","unit":"nm"}],"datasets":[{"coordinateTransformations":[{"scale":["8","8","8"],"type":"scale"}],"path":"s0"},
{"coordinateTransformations":[{"scale":["88","88","88"],"type":"scale"}],"path":"s1"},
{"coordinateTransformations":[{"scale":["8888","8888","8888"],"type":"scale"}],"path":"s2"},
{"coordinateTransformations":[{"scale":["88888888","88888888","88888888"],"type":"scale"}],"path":"s3"},{"coordinateTransformations":[{"scale":["8888888888888888","8888888888888888","8888888888888888"],"type":"scale"}],"path":"s4"},
{"coordinateTransformations":[{"scale":["88888888888888888888888888888888","88888888888888888888888888888888","88888888888888888888888888888888"],"type":"scale"}],"path":"s5"},
...
],"name":"ppp","version":"0.4"}]}

The project validates correctly, but in the viewer things go nuts ;-)

Should we check for the pixel size being numeric? Or would you rather change the way it writes the downsampled scale? The array data is fine. It is really just the metadata that's screwed.

martinschorb commented 8 months ago

It even accepts a list of empty strings for the pixel size. This leads to a crash in the viewer but still a properly validating source.

martinschorb commented 7 months ago

I think this is where it happens:

https://github.com/constantinpape/cluster_tools/blob/366f2a1a57d5c01a7d51548c74e1d53465a25924/cluster_tools/utils/volume_utils.py#L517

it's the * operator acting on str instead of a number.

@constantinpape I would suggest adding a conversion of a potential str to float maybe one level higher in write_format_metadata and a type check throwing an error if that conversion does not succeed.

any objections? I will make a PR otherwise.

martinschorb commented 7 months ago

with the schema https://github.com/ome/ngff/blob/main/0.4/schemas/image.schema asking for a number:

"coordinateTransformations": {
      "type": "array",
      "minItems": 1,
      "contains": {
        "type": "object",
        "properties": {
          "type": {
            "type": "string",
            "enum": [
              "scale"
            ]
          },
          "scale": {
            "type": "array",
            "minItems": 2,
            "items": {
              "type": "number"
            }
          }

it means that strings are not meant to end up in there anyway (even though the Fiji Viewer seems to load them).

Do you have any plans to include a metadata validation for the sources? This would probably have caught here.

constantinpape commented 7 months ago

@constantinpape I would suggest adding a conversion of a potential str to float maybe one level higher in write_format_metadata and a type check throwing an error if that conversion does not succeed.

Yes, that's a good idea. Please go ahead with a PR!

constantinpape commented 7 months ago

Do you have any plans to include a metadata validation for the sources? This would probably have caught here.

Yes, we should include it. I don't have time to look into this myself now, but a PR on that would be very welcome.

The best place to add it would be here: https://github.com/mobie/mobie-utils-python/blob/master/mobie/validation/metadata.py#L62

martinschorb commented 7 months ago

fixed with above PRs.