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.

Sign up for GitHub

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

Jump to bottom

Add process to automatically download data from MIBItracker prior to QC analysis #483

Merged
merged 25 commits into from
Dec 7, 2021
Merged

Add process to automatically download data from MIBItracker prior to QC analysis #483

merged 25 commits into from
Dec 7, 2021

Conversation

alex-l-kong
Copy link
Contributor

What is the purpose of this PR?

Addresses and closes #482.

How did you implement your changes

Please read the design doc in the link to the issue for more info.

@alex-l-kong alex-l-kong self-assigned this Nov 19, 2021
@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
Copy link
Contributor Author

Coverage has gone down because we do not test mibitracker_utils. This is just a hotfix while we wait for Ionpath to resolve their pinning issue in using their library.

ngreenwald
ngreenwald requested changes Nov 23, 2021
View reviewed changes
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.

Nice start, minor comments only

ark/mibi/qc_comp.py Show resolved Hide resolved
ark/mibi/qc_comp.py Show resolved Hide resolved
ark/mibi/qc_comp.py Show resolved Hide resolved
ark/mibi/qc_comp.py Outdated Show resolved Hide resolved
ark/mibi/qc_comp.py Show resolved Hide resolved
@review-notebook-app
Copy link

review-notebook-app bot commented Nov 23, 2021 •
edited
Loading

View / edit / reply to this conversation on ReviewNB

ngreenwald commented on 2021-11-23T00:54:59Z
----------------------------------------------------------------

I think having this file in templates will be confusing. Let's move the MIBI-specific notebooks to a separate folder.


alex-l-kong commented on 2021-11-23T18:02:58Z
----------------------------------------------------------------

Sounds good.

@review-notebook-app
Copy link

review-notebook-app bot commented Nov 23, 2021 •
edited
Loading

View / edit / reply to this conversation on ReviewNB

ngreenwald commented on 2021-11-23T00:55:00Z
----------------------------------------------------------------

Which projects does this account have access to?


alex-l-kong commented on 2021-11-23T17:48:32Z
----------------------------------------------------------------

It connects to Jaiswal's data (Mouse Heart: Aortic Root). Any reason we can't use this dataset for testing?

@review-notebook-app
Copy link

review-notebook-app bot commented Nov 23, 2021 •
edited
Loading

View / edit / reply to this conversation on ReviewNB

ngreenwald commented on 2021-11-23T00:55:00Z
----------------------------------------------------------------

Line #8.        save_dir=None

Is there an option to save the data file in addition to saving the plots for these functions?


alex-l-kong commented on 2021-11-23T17:48:58Z
----------------------------------------------------------------

That's done in a prior cell in the notebook (the one commented 'save the QC metric values').

@alex-l-kong Review Notebook App
Copy link
Contributor Author

It connects to Jaiswal's data (Mouse Heart: Aortic Root).

View entire conversation on ReviewNB

@alex-l-kong Review Notebook App
Copy link
Contributor Author

That's done in a prior cell in the notebook.

View entire conversation on ReviewNB

@alex-l-kong Review Notebook App
Copy link
Contributor Author

Sounds good.

View entire conversation on ReviewNB

ackagel
ackagel requested changes Nov 24, 2021
View reviewed changes
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.

Looks largely fine to me; basically just one security thing

ark/mibi/qc_comp.py Show resolved Hide resolved
ark/mibi/qc_comp.py Outdated Show resolved Hide resolved
ark/mibi/qc_comp.py Show resolved Hide resolved
ark/mibi/qc_comp_test.py Show resolved Hide resolved
ngreenwald
ngreenwald requested changes Nov 25, 2021
View reviewed changes
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.

Looking good!

start_docker.sh Show resolved Hide resolved
@ngreenwald ngreenwald merged commit c7daee5 into master Dec 7, 2021
@ngreenwald ngreenwald deleted the download_mibi branch December 7, 2021 00:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ngreenwald ngreenwald ngreenwald approved these changes

@efrancis28 efrancis28 Awaiting requested review from efrancis28

@ackagel ackagel Awaiting requested review from ackagel

Assignees

@alex-l-kong alex-l-kong

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add automatic MIBI image downloading from MIBItracker for QC analysis
3 participants
@alex-l-kong @ngreenwald @ackagel