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

Minor issues in milopy tools #181

Open
emdann opened this issue Nov 15, 2022 · 6 comments
Open

Minor issues in milopy tools #181

emdann opened this issue Nov 15, 2022 · 6 comments
Assignees

Comments

@emdann
Copy link
Member

emdann commented Nov 15, 2022

Noting down issues to fix and potential improvements to milopy, encountered while writing tutorial (scverse/pertpy-tutorials#4) - issues entirely inherited from my code ofc :)

milo.add_covariate_to_nhoods_var

  • change name to add_covariate_to_nhoods_obs and update docs
  • Check types of added columns (e.g. can go from category to object and then nhood_counts_by_cond complains

pt.pl.milo.nhood_counts_by_cond

  • add informative error for when covariate is missing (directing to use add_covariate_to_nhoods_var)
  • convert strings to categoricals internally (or add informative errors for dtype object)

milo.da_nhoods:

  • at the moment the use of subset_samples and subset_nhoods is buggy, in some edge cases this messes up the SpatialFDR calculation. While the bug is easy enough to catch with diagnostic plots (randomized SpatialFDR) and to fix (Misc bug fixes emdann/milopy#30), I'm wondering whether these parameters should be dropped completely in the new implementation in pertpy. The use case of subset_samples is essentially covered by specifying contrasts (+ adding a new column in adata.obs if needed to specify groups). Using subset_nhoods is somewhat of a pathological hack (I don't think I've ever used it and I can't even remember the original use case for adding this parameter). In practice if one wants to test on a specific subset of cell phenotypes the best solution is to subset cells and restart from KNN graph building.
  • Add informative errors for missing columns in model matrix (i.e. when the reference level is specified in contrasts)

Happy to open a PR to implement these changes.

@Zethson
Copy link
Member

Zethson commented Nov 15, 2022

@emdann I'd be in favor of dropping the subset_samples and subset_nhoods parameters. Do you think that there's a reasonable place and a need somewhere to mention this though? Like maybe in the docs?

@emdann
Copy link
Member Author

emdann commented Nov 15, 2022

Hopefully the new tutorial covers the use of contrasts to specify comparisons. I could add an example on subsetting to a certain lineage or something.

@xinyuejohn
Copy link
Collaborator

@emdann It would also be great to add plot_milo_diagnostics into milopy. I found this function is pretty helpful and has been used for several times in best-practice notebook.

@Zethson
Copy link
Member

Zethson commented Feb 1, 2023

@emdann do you think that you'd have time to tackle this soonish?

@emdann
Copy link
Member Author

emdann commented Feb 1, 2023

In principle I agree with adding the diagnostic plots to the package, although in my head that's more material for extended tutorials since it needs some guidance for interpretation. But up to you guys to decide what fits in the package and what doesn't!

Unfortunately I am going to be slumped in response-to-reviewer work for the next ~ 4 weeks.

@Zethson
Copy link
Member

Zethson commented Nov 20, 2023

@emdann let's tackle this at the hackathon, OK?

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

No branches or pull requests

3 participants