search

mlexchange / mlex_dlsia_segmentation_prototype

Other
3 stars 3 forks source link

Add unit test and refactor TiledDataset #23

Closed dylanmcreynolds closed 7 months ago

dylanmcreynolds commented 7 months ago

This PR adds a unit test that creates a database and runs it in github actions. The unit test starts up a tiled server and connects to it.

It also slightly refactors the TiledDataset, using dependency injection to send tiled clients as init parameters rather than having it construct its own internally. This makes this type of unit testing easier. Without that, then we would have to monkey patch the TiledDataset and its data_client and mask_client members. That's possible, but dependency injection is more flexible and readable.

TibbersHao commented 7 months ago

Thanks for the hard work! I did several rounds of the test on my local end, and would suggest a few more changes with the introduction of the dependency injection, most of them are related to the additional layer of container for the actual masks, (referring to the "one_up_container" we discussed back on Tuesday).

And for the unit tests, I ran into one error and it seems to be related to the import of from_uri. I will address this in the detailed code review too.

dylanmcreynolds commented 7 months ago

I think this is ready for you to test again. I refactored the test methods. conftest.py is a module that pytest knows automatically and applies to all tests. So, the fixtures I built can be used by multiple (future) test modules. I also merged in the recent changes to dlsia requirements. So when you test, pip install requirements.txt an then requirements-dev.txt in that order.

TibbersHao commented 7 months ago

I think this is ready for you to test again. I refactored the test methods. conftest.py is a module that pytest knows automatically and applies to all tests. So, the fixtures I built can be used by multiple (future) test modules. I also merged in the recent changes to dlsia requirements. So when you test, pip install requirements.txt an then requirements-dev.txt in that order.

Thanks for the modification. Now pytest works fine on my end, and all checks passed. In order to execute train.py and segment.py properly, I made 2 minor commits to bring back the functionality of allocate_array_space as a reflection of the new TiledDataset. Tested with the clay dataset from SPIN, it worked on my end.

Unless @taxe10 and @Wiebke have additional thoughts, I believe this PR is ready to be merged.