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 unit test and refactor TiledDataset #23

Merged
merged 22 commits into from
Apr 15, 2024
Merged

Add unit test and refactor TiledDataset #23

merged 22 commits into from
Apr 15, 2024

Conversation

dylanmcreynolds
Copy link
Member

@dylanmcreynolds dylanmcreynolds commented Apr 9, 2024

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

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.

src/segment.py Outdated Show resolved Hide resolved
src/tiled_dataset.py Outdated Show resolved Hide resolved
src/train.py Outdated Show resolved Hide resolved
src/train.py Outdated Show resolved Hide resolved
src/train.py Outdated Show resolved Hide resolved
Copy link
Member

@TibbersHao TibbersHao left a comment

Choose a reason for hiding this comment

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

I've added a few comments with code suggestions to accommodate the dependency injection change.

Also I do have one more suggestion to request for the change of allocate_array_space function in utils.py, but I was not able to find a way to point it out in this review as no prior changes of that file have been made in this PR. Please let me know what's a good way to point it out.

@dylanmcreynolds
Copy link
Member Author

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

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.

@TibbersHao TibbersHao merged commit e1d57e6 into main Apr 15, 2024
2 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.

3 participants