search

ebi-gene-expression-group / monocle-scripts

Command line scripts wrapping functions in monocle3
8 stars 1 forks source link

monocle top markers #12

Closed pcm32 closed 4 years ago

pcm32 commented 4 years ago

This PR adds the markers test and markers filtering functionality to produce top markers in monocle. There are a few things that I'm not sure how they work in the current layout of the package, for instance whether I need to do something to inhering the output_plot and output_plot_format, or how to deal with the case where the method can output more than a single table (looking to inherit the output_table option).

@nh3 could you advise on what I'm missing to complete this? Thanks!

pcm32 commented 4 years ago

Also, I think that I'm using some dplyr constructs, is there a way to specify a library call for this particular function (I could call it inside the function, but I wonder if you are using some other way). dplyr is part of the conda package for monocle3-cli.

pcm32 commented 4 years ago

I have removed as much as possible redundant travis.org and travis.com (to leave only the last one for PRs), to reduce our carbon footprint :-).

nh3 commented 4 years ago

Many thanks for the PR @pcm32! I can do the rest. Just to confirm, you want it to always write a full marker table, an optional top marker table and an optional marker-cluster plot, is that right?

pcm32 commented 4 years ago

Yes, sounds right, unless that you think that top markers output should be the relevant one and the full one the optional. Both ways work for me. Thanks @nh3! I can do release and so on later if you want.

pcm32 commented 4 years ago

Hehehe, apparently I was a bit off on how to implement this.

nh3 commented 4 years ago

The inner workings aren't straightforward and I didn't write documentation, it took me sometime too. In the end, I chose to save top markers as a mandatory output.

I think this is now ready for review.

pcm32 commented 4 years ago

Being the author of the PR, I cannot review it. @pinin4fjords or @a-solovyev12 could you please take a look? Thanks @nh3 for the changes, it looks good to me at least.

nh3 commented 4 years ago

Thank you @pinin4fjords and @pcm32. Is there a way to make travis CI run the other tests? Otherwise it seems it wouldn't allow us to merge without force.

pinin4fjords commented 4 years ago

Try pushing a dummy spacing change or something @nh3 ? Has worked in previous situations where Travis got confused.

nh3 commented 4 years ago

Thanks @pcm32! Actually I wonder how did you do this. Was always a bit tired to see the same tests run 2-3 times...

I have removed as much as possible redundant travis.org and travis.com (to leave only the last one for PRs), to reduce our carbon footprint :-).

pcm32 commented 4 years ago

I went to the travis-ci.org and .com dashboard, once I was sure that this was active in the travis-ci.com side, I turned off the travis-ci.org ones. Then I only activated the PR one on the travis-ci.com (turned off the branch one). But we can use branch if you want instead of PR, I just wouldn't use both.