ebi-gene-expression-group / container-galaxy-sc-tertiary

Galaxy container for single cell RNA-Seq tertiary analysis tools
MIT License
16 stars 13 forks source link

Scanpy FilterCell wrapper fails with current version #84

Open pinin4fjords opened 5 years ago

pinin4fjords commented 5 years ago

@nh3 - seems to be a bit of an issue with the FilterCells tool for Scanpy. You can't use 'n_counts' or 'n_genes'- seems to need to be cell:n_counts or cell:n_genes. Otherwise you get:

ERROR:root:Ambiguous parameter name [n_counts] given, choose from "gene:{n_counts}" or "cell:{n_counts}"
Traceback (most recent call last):
  File "/nfs/production3/ma/home/pmoreno/_c_dev/envs/__scanpy-scripts@0.2.4.post4/bin/scanpy-filter-cells", line 6, in <module>
    exit(FILTER_CMD())
  File "/nfs/production3/ma/home/pmoreno/_c_dev/envs/__scanpy-scripts@0.2.4.post4/lib/python3.6/site-packages/click/core.py", line 764, in __call__
    return self.main(*args, **kwargs)
  File "/nfs/production3/ma/home/pmoreno/_c_dev/envs/__scanpy-scripts@0.2.4.post4/lib/python3.6/site-packages/click/core.py", line 717, in main
    rv = self.invoke(ctx)
  File "/nfs/production3/ma/home/pmoreno/_c_dev/envs/__scanpy-scripts@0.2.4.post4/lib/python3.6/site-packages/click/core.py", line 956, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/nfs/production3/ma/home/pmoreno/_c_dev/envs/__scanpy-scripts@0.2.4.post4/lib/python3.6/site-packages/click/core.py", line 555, in invoke
    return callback(*args, **kwargs)
  File "/nfs/production3/ma/home/pmoreno/_c_dev/envs/__scanpy-scripts@0.2.4.post4/lib/python3.6/site-packages/scanpy_scripts/cmd_utils.py", line 40, in cmd
    func(adata, **kwargs)
  File "/nfs/production3/ma/home/pmoreno/_c_dev/envs/__scanpy-scripts@0.2.4.post4/lib/python3.6/site-packages/scanpy_scripts/lib/_filter.py", line 48, in filter_anndata
    adata, attributes, param, category, subset)
  File "/nfs/production3/ma/home/pmoreno/_c_dev/envs/__scanpy-scripts@0.2.4.post4/lib/python3.6/site-packages/scanpy_scripts/lib/_filter.py", line 166, in _get_filter_conditions
    raise ValueError
ValueError

I assume this is because they're calling the same function? But that should probably be hidden from the user.

nh3 commented 5 years ago

For "n_counts", because it can be associate with both cells and genes, so the users need to specify whether they want cell or gene. For "n_genes", it's not necessary as it's solely associated with cells. This is one of quite a few interface changes you will notice in 0.2.x.

pinin4fjords commented 5 years ago

@nh3 But this tool is just cell filtering, no? So we're filtering by number of genes or number of reads (cell:n_genes, cell:n_counts). How is gene:n_counts relevant?

nh3 commented 5 years ago

There actually is only one tool called "filter" in the scripts layer, which do all kinds of filtering in one step. Two aliases (filterCell, filterGene) of it were created just to make the command names compatible with 0.0.x.

nh3 commented 5 years ago

By the way, if you actually use the galaxy interface "filterCell", when you type in that box, it will pop up choices like "n_counts" and "n_genes", and if you click "n_counts" it will automatically change to "cell:n_counts". I thought that's sufficient for end users.

pinin4fjords commented 5 years ago

@nh3 yep, I get that. My point is, the user shouldn't need to care. They (and I!) will fairly logically expect that using a tool called 'Filter Cells' that they're filtering cells. I don't think the 'cell:' should be visible to them anywhere, we can just pass it internally to the wrapper, no?

pinin4fjords commented 5 years ago

What do you think @pcm32 ?

pcm32 commented 5 years ago

I guess that if filter gets an action passed by filterCells then that process should pre-pend whatever is needed to accomodate that. For the current version we could handle that within the Galaxy wrapper I guess, but I agree that the user shouldn't need to be aware of this things if it is directly calling a function that implies its intention explicitly on its name. If the user would be calling filter directly, I agree it would need to give precisions on what he/she intends to filter.

pinin4fjords commented 5 years ago

Would you like to tweak the wrapper to that effect @nh3 - or should I just have go and make a PR?