Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add test for 2D CLI #128

Merged
merged 16 commits into from
Feb 14, 2024
Merged

add test for 2D CLI #128

merged 16 commits into from
Feb 14, 2024

Conversation

martinschorb
Copy link
Contributor

this addresses #127

Test fails to demonstrate the issue. WIP

@martinschorb
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@constantinpape
Copy link
Contributor

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

@martinschorb
Copy link
Contributor Author

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

Copy link
Contributor

@constantinpape constantinpape left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look good now. But the default data format should also be changed for other functions, e.g. https://github.com/mobie/mobie-utils-python/blob/master/mobie/segmentation.py#L20.

But we can also do this in a separate PR and merge this one now.

@martinschorb
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor

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

@constantinpape
Copy link
Contributor

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
Copy link
Contributor Author

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

@constantinpape constantinpape merged commit b4a7de6 into master Feb 14, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants