search

ebi-gene-expression-group / scanpy-scripts

Scripts for using scanpy
Apache License 2.0
29 stars 13 forks source link

Remove CLI-level n_pcs hack and allow fallback to Scanpy's #89

Closed pinin4fjords closed 3 years ago

pinin4fjords commented 3 years ago

This PR was prompted because a previous PCA hack (mine I think) at https://github.com/ebi-gene-expression-group/scanpy-scripts/blob/20b526355b0125b01062dbcad79e07e84a5db0f3/scanpy_scripts/lib/_pca.py#L18 stopped working with newer Scanpy (or maybe it's dependencies)- it would now need to be (adata.n_obs - 1). But actually I think it would be better to allow triggering of Scanpy's fallback here: https://github.com/theislab/scanpy/blob/6316d33640c44c9f9f92b655bbc601b54c126937/scanpy/preprocessing/_pca.py#L145, which we can only do by allowing non-specification of the n_pcs parameter.

nh3 commented 3 years ago

The changes look fine to me. Strange that this PR doesn't trigger CI tests. Please feel free to merge after the test is done.

nh3 commented 3 years ago

hmm, it doesn't require a passing test to merge it seems.

pinin4fjords commented 3 years ago

Thanks @nh3 . We're out of Travis credits (they changed their accounting for open source projects recently)

pcm32 commented 3 years ago

Maybe we should wait until we have tests back to avoid introducing issues.

pcm32 commented 3 years ago

or move them to github actions.

pinin4fjords commented 3 years ago

New Actions tests are passing, merging now.