search

mobie / mobie-utils-python

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

add test for 2D CLI #128

Closed martinschorb closed 7 months ago

martinschorb commented 7 months ago

this addresses #127

Test fails to demonstrate the issue. WIP

martinschorb commented 7 months ago

Is it OK if we just choose ome.zarr as default? I guess by now, that would be my call.

Or will it break some older scripts that assume bdv.n5 to be in there as default for all eternity?

martinschorb commented 7 months ago

Maybe this would then justify an increase in version number (minor only). What do you think @constantinpape ?

constantinpape commented 7 months ago

I agree @martinschorb. We should switch the default and then bump the minor version.

martinschorb commented 7 months ago

looks like there are a bunch of tests relying on the default. Will look into it and try to fix them.

martinschorb commented 7 months ago

I found that this test is full of explicit bdv.n5 calls. https://github.com/mobie/mobie-utils-python/blob/master/test/metadata/test_source_metadata.py

But I don't fully understand the way these tests work and if the format plays a role at all for them.

constantinpape commented 7 months ago

I found that this test is full of explicit bdv.n5 calls.

The reason is that this test checks for the json validation of the source field and has some specific settings for bdv.n5, see https://github.com/mobie/mobie-utils-python/blob/master/test/metadata/test_source_metadata.py#L19-L22

I think we can leave this test as it is.

constantinpape commented 7 months ago

I have updated the remaining places where the default file format was still bdv.n5. I will look into the failing tests later.

constantinpape commented 7 months ago

Just fyi @martinschorb, there is some additional logic needed to support ome.zarr for all the scripts. This is in principle not a problem, but I am not 100% sure when I have time to implement it. If you think we should get this out as soon as possible we can also skip this and do the update already. (This is only for some rather obscure functionality, so I think it would not affect any users.)

martinschorb commented 7 months ago

I am totally fine with some functionality being restricted to (legacy) file formats. For the sorresponding tests, I would just explicitely specify the format.