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

Implementation of CINEMA-OT for pertpy #377

Merged
merged 9 commits into from
Sep 15, 2023
Merged

Conversation

MingzeDong
Copy link
Contributor

Implementation of CINEMA-OT using the ott-jax library. Including an updated usage.md, tl.Cinemaot class and CinemaotPlot class. A test file is included. Let me know if there are any issues.

Copy link
Member

@Zethson Zethson left a comment

Choose a reason for hiding this comment

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

Thank you very much for the really fast contribution!

  1. I made a couple of minor corrections and removed unused imports and parameters. Feel free to checkout my last commit if I deleted a parameter that you actually wanted to implement.
  2. I've uploaded your test dataset to our figshare and wrote a dataloader for it.
  3. I rewrote your tests with a fixture.
  4. FAILED tests/tools/test_cinemaot.py::TestCinemaot::test_pseudobulk - TypeError: Setting a MultiIndex dtype to anything other than object is not supported This test seems to fail? Could you please try to fix it?
  5. In an ideal world, the tests would also test not just that the code runs but that the results are also within some expected range.

@Zethson
Copy link
Member

Zethson commented Sep 15, 2023

@MingzeDong I'll merge this today but it'd be really awesome if you could submit a follow up PR with a few improved tests and the failing test being fixed.

adata = pt.dt.cinemaot_example() now works. Please use that for the tutorial notebook.

Thank you so much!

@Zethson Zethson merged commit 7b1314f into scverse:main Sep 15, 2023
2 of 5 checks passed
@MingzeDong
Copy link
Contributor Author

Thanks! I have submitted a follow up PR and will submit a tutorial notebook later.

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.

2 participants