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 MIBI tiling script #427

Merged
merged 27 commits into from
Aug 10, 2021
Merged

Add MIBI tiling script #427

merged 27 commits into from
Aug 10, 2021

Conversation

alex-l-kong
Copy link
Contributor

@alex-l-kong alex-l-kong commented Jul 8, 2021

What is the purpose of this PR?

Begins addressing #413. We start by adding a tiling script to ark allowing users to generate tiled regions across fovs for a given run.

How did you implement your changes

We add example_fov_grid_generate.ipynb, which runs the tiling process. A new repo, ark/mibi, will contain helper functions. So far, only tiling_utils.py has been added to encapsulate the tiling process.

Additionally, sample JSON data has been added to data/example_dataset.

Remaining issues

Testing still needs to be addressed.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@alex-l-kong alex-l-kong self-assigned this Jul 8, 2021
@alex-l-kong
Copy link
Contributor Author

The tests have yet to be added in, but I would appreciate an initial review of the overall structure. I'll begin tagging Erin on this PR after more progress.

Copy link
Member

@ngreenwald ngreenwald left a comment

Choose a reason for hiding this comment

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

Good start, see below for some initial comments

ark/mibi/tiling_utils.py Outdated Show resolved Hide resolved
ark/mibi/tiling_utils.py Outdated Show resolved Hide resolved
ark/mibi/tiling_utils.py Outdated Show resolved Hide resolved
@alex-l-kong
Copy link
Contributor Author

alex-l-kong commented Jul 27, 2021

I'm researching ways to test set_tiling_params because those require input arguments from the user. In the meantime, a test for create_tiled_regions has been added.

UPDATE: the monkeypatch feature of pytest provides a nice way to handle this.

Copy link
Member

@ngreenwald ngreenwald left a comment

Choose a reason for hiding this comment

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

Looks good, some more tweaks

ark/mibi/tiling_utils.py Outdated Show resolved Hide resolved
ark/mibi/tiling_utils.py Outdated Show resolved Hide resolved
ark/mibi/tiling_utils.py Outdated Show resolved Hide resolved
ark/mibi/tiling_utils.py Outdated Show resolved Hide resolved
ark/mibi/tiling_utils.py Outdated Show resolved Hide resolved
ark/mibi/tiling_utils.py Outdated Show resolved Hide resolved
Copy link
Member

@ngreenwald ngreenwald left a comment

Choose a reason for hiding this comment

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

Looks good. The testing needs to be refactored.

ark/mibi/tiling_utils.py Outdated Show resolved Hide resolved
ark/mibi/tiling_utils_test.py Outdated Show resolved Hide resolved
ark/mibi/tiling_utils_test.py Outdated Show resolved Hide resolved
ark/mibi/tiling_utils_test.py Outdated Show resolved Hide resolved
Copy link
Contributor

@ackagel ackagel left a comment

Choose a reason for hiding this comment

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

Just some thoughts on the tiling_params structure. Agree with Noah's point on condensing/generalizing the test functions.

ark/mibi/tiling_utils.py Outdated Show resolved Hide resolved
ark/mibi/tiling_utils.py Outdated Show resolved Hide resolved
ark/mibi/tiling_utils.py Outdated Show resolved Hide resolved
Copy link
Contributor

@bcollica bcollica left a comment

Choose a reason for hiding this comment

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

Just minor stuff

ark/mibi/tiling_utils.py Outdated Show resolved Hide resolved
ark/mibi/tiling_utils_test.py Outdated Show resolved Hide resolved
Copy link
Contributor

@bcollica bcollica left a comment

Choose a reason for hiding this comment

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

Looks amazing! My only thought would be that set_tiling_params() is a bit long at 125 lines, mostly due to the repetitive nature of the calls to read_tiling_param(). Breaking those calls out as their own separate functions might help condense it, but not sure if it's really worth the trouble?

Copy link
Member

@ngreenwald ngreenwald left a comment

Choose a reason for hiding this comment

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

Looks good!

@alex-l-kong alex-l-kong merged commit 823ecb5 into master Aug 10, 2021
@alex-l-kong alex-l-kong deleted the tiling_add branch August 10, 2021 17:20
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.

4 participants