8000 Packaging by mumichae · Pull Request #272 · theislab/scib · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Packaging #272

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

Merged
merged 53 commits into from
Oct 24, 2021
Merged

Packaging #272

merged 53 commits into from
Oct 24, 2021

Conversation

mumichae
Copy link
Collaborator
@mumichae mumichae commented Oct 3, 2021

Add functionality to compute trajectory score per batch and take the mean and not compute the score overall.

@mumichae mumichae self-assigned this Oct 3, 2021
@mumichae mumichae marked this pull request as ready for review October 7, 2021 14:35
@mumichae mumichae requested a review from LuckyMD October 7, 2021 14:35
mumichae and others added 12 commits October 7, 2021 19:06
* add per batch trajectory score

* Update trajectory.py

add missing comma

* Update trajectory.py

import pandas

* don't recompute trajectories per batch

* correct batch var handling

* Update trajectory.py

* Check batch key

* add tests for trajectory score

* update test values

* update test values

Co-authored-by: Strobl <daniel.strobl@mb084184.dyn.scidom.de>
Co-authored-by: Michaela Mueller <mumichae@in.tum.de>
Copy link
Collaborator
@LuckyMD LuckyMD left a comment

Choose a reason for hiding this comment

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

Looks great! Just a few comments, which mostly will be due to my lack of understanding ;)

Copy link
Collaborator
@LuckyMD LuckyMD left a comment

Choose a reason for hiding this comment

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

Overall this looks very nice. Just two main points:

  1. Do we have a parallel PR in scib-pipeline to ensure all of the scripts are using the correct function names? Or are we running our own module in a deprecated manner by default?
  2. @danielStrobl can we assign batch in the trajectory score as you did? Why should this not already be in the second object?

@mumichae mumichae requested a review from LuckyMD October 22, 2021 12:58
Copy link
Collaborator
@LuckyMD LuckyMD left a comment

Choose a reason for hiding this comment

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

Looks great! If you have tested the traj cons batch on scanorama and mnn, then this is good to go!

@mumichae mumichae merged commit 985d815 into master Oct 24, 2021
@mumichae mumichae deleted the packaging branch October 24, 2021 16:44
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
0