Update STAR version + new options for STARsolo #5060

lldelisle · 2023-01-16T21:06:46Z

FOR CONTRIBUTOR:

- I have read the CONTRIBUTING.md document and this tool is appropriate for the tools-iuc repo.
- License permits unrestricted use (educational + commercial)
- This PR adds a new tool or tool collection
- This PR updates an existing tool or tool collection
- This PR does something else (explain below)

lldelisle · 2023-01-17T09:13:42Z

@mtekman , @wm75, @JasperO98 , @pavanvidem, @ieguinoa
If you are interested in reviewing...
Thanks

tools/rgrnastar/rg_rnaStarSolo.xml

lldelisle · 2023-01-17T10:11:11Z

Just to be clear:

I did not updated the bam for the STAR tests (the sizes were matching and passing tests).
When using outSAMattributes CB or UB, the way the bam is sorted is changed but I did not make it appear to the user.

mtekman

Looks good on my side

tools/rgrnastar/rg_rnaStarSolo.xml

pavanvidem · 2023-01-17T11:28:01Z

@lldelisle I can look at it in the evening.

I have the following features/params/UI rearrangements that I have in my TODO for STARSolo.

Smart-seq mapping sometimes fails with

EXITING because of fatal error: buffer size for SJ output is too small
Solution: increase input parameter --limitOutSJcollapsed

this can be fixed with adding --limitOutSJcollapsed param

Allow compatibility with CEL-seq. The main difference to 10x that I see is that we do not have a barcode whitelist. Instead, we have lengths of barcodes and UMIs in CEL-seq. Check this issue for an example call. We can already build that command with custom chemistry options in our current wrapper. But we have to make --soloCBwhitelist optional and adapt the cheetah.
When filtering is turned on, we should also additionally output the non-filtered data. At the moment, it only outputs the filtered matrix. This does not allow users to try other filtering options for example with DropletUtils.
Organize the UI better. Some options are not compatible with all protocols. For example, I am not sure whether --soloUMIfiltering is used for Smart-seq.

Do you think is it reasonable to add (some of) them to this PR?

lldelisle · 2023-01-17T12:26:57Z

@pavanvidem sure.
Do you want to write it or do you want me to write it?

lldelisle · 2023-01-17T12:28:42Z

I just realize that the Chemistry parameter propose "Cell Ranger v2" while Cell Ranger is the name of the software, for me it should be "Chromium chemistry v2"...

pavanvidem · 2023-01-17T12:40:46Z

@pavanvidem sure. Do you want to write it or do you want me to write it?

I will work on it. If you wish please contribute :)

pavanvidem · 2023-01-17T12:43:38Z

I just realize that the Chemistry parameter propose "Cell Ranger v2" while Cell Ranger is the name of the software, for me it should be "Chromium chemistry v2"...

Good cath, it is correct.

lldelisle · 2023-01-18T07:13:20Z

@pavanvidem , here is what I have in mind, tell me if this would work for you:

For --limitOutSJcollapsed, I can just take the block limits from STAR to STARsolo

CEL-seq, I see 2 possibilities:

We add a new solo_type "CEL-seq" which would use:

--soloType CB_UMI_Simple 
--soloCBstart 7
--soloCBlen 6  
--soloUMIstart 1  
--soloUMIlen 6  
--soloBarcodeMate 2 
--soloCBwhitelist None
--clip5pNbases 0 36

Or we simply set soloCBwhitelist optional in CB_UMI_Simple, this is more flexible.

For the Counts/UMI outputs, I think most of the case people don´t really want to see the counts, I propose that we add a parameter to make them appear.
For --soloUMIfiltering you are right this is only compatible with –soloUMIdedup 1MM CR. I can make a conditional.

pavanvidem · 2023-01-18T12:26:03Z

True, there are already Limits in STAR. Maybe not all limits are useful, but it doesn't hurt to add them too. Then we can move Limits block to macros.
I like the 2nd option. With the first option, we're not flexible in choosing barcode and UMI lengths.
Then, we can add a check box to make the unfiltered results to appear.
Perfect

FYI, I just started working on the above options sequentially.

lldelisle · 2023-01-18T12:28:42Z

Perfect, tell me if you need help.

make `--soloCBwhitelist` optional and add limits macro for Smart-Seq

bernt-matthias · 2023-01-26T14:37:19Z

Would it be possible to include

STAR: allow fasta.gz for reference #5101 (which I just opened :( )
Add limits also to star solo #4285

Maybe also check if those two old issues are still relevant:

Enhancement: Add "--outWigType" && "--outWigStrand" options to RNA STAR too wrapper #1742
RNA STAR allows job submission when required inputs are missing, results in obscure cluster error #1777

lldelisle · 2023-01-26T14:40:50Z

Good catch @bernt-matthias , thanks. Can you target your new PR to my branch?
@pavanvidem where are your with your implementation?

pavanvidem · 2023-01-26T14:59:51Z

Good catch @bernt-matthias , thanks. Can you target your new PR to my branch? @pavanvidem where are your with your implementation?

last week I created a patch on your fork: lldelisle#2

bernt-matthias · 2023-01-26T16:19:12Z

I just pushed 033cd4a .. hape that is fine

tools/rgrnastar/rg_rnaStar.xml

plus: - removed a few redundant name attribs - increases profile

pavanvidem

@lldelisle Fantastic! I have a final comment on cell filtering. Does it make sense to allow droplet-based filtering for Smart-Seq? Technically it is possible. But do people use this option? Maybe I'm trying to optimize too much. I'm already very happy with the current state of the wrapper and it can be merged. Thanks a lot for your efforts!!

lldelisle · 2023-02-15T11:35:32Z

Maybe you are right, it does not make a lot of sense but then this suppose to put the conditional block filter inside the conditional block sc instead of the advanced setting. I don't mind doing it but I feel like it is ok to keep all options even if the user selected 'Smart-seq'. Tell me what you prefer.

pavanvidem · 2023-02-15T11:42:29Z

As I said, it is all good and It does not break anything. Ready to merge from my side.

lldelisle · 2023-02-15T11:45:22Z

@bgruening I think @pavanvidem is not a 'member' and his approval does not allow me to merge the PR. Could you approve it or you prefer a second review?

wm75

Just a bit of (important) housekeeping.
For everything else I trust you and other reviewers :)

tools/rgrnastar/macros.xml

tools/rgrnastar/rg_rnaStar.xml

tools/rgrnastar/rg_rnaStarSolo.xml

wm75 · 2023-02-16T10:47:15Z

tools/rgrnastar/macros.xml

@@ -5,7 +5,8 @@
    the index versions in sync, but you should manually adjust the +galaxy
    version number. -->


tools-iuc/data_managers/data_manager_star_index_builder/data_manager/rna_star_index_builder.xml

Line 1 in 02d1d48

<tool id="rna_star_index_builder_data_manager" name="rnastar index versioned" tool_type="manage_data" version="@IDX_VERSION@" profile="19.05">

is what this comment refers to.
This PR should, because of the linked macros file, trigger deployment of a new version of the DM, too, so you need to bump the DM version to version="@IDX_VERSION@+galaxy1"

You mean we should change the version because we changed STAR version or because of something else?

I guess bumping the IDX_VERSION_SUFFIX does not hurt, but I do not understand why we should do it?

Maybe to indicate that we use the STAR version @TOOL_VERSION@ instead of the STAR version @IDX_VERSION@ to build the index...

@bernt-matthias @lldelisle Just to explain things: the reason for symlinking the macros file is to keep the IDX_VERSION in one place only so that when you update the tool wrapper to a STAR version that requires a newer index format, you'd automatically deploy also a DM that can create these indexes.

The "downside" is that any changes to the tool wrapper macros file will silently affect the DM. So in this case the next version of the DM will use the 2.7.10b version of star for building indexes. These should be identical to ones built with older versions, but it's good to bump the DM wrapper version to be able to trace things back.

Seems legit.

Could a more expressive filter help here. For instance we could just store the star version that was used to create an entry in the datatable ... and then just filter datatable entries for a min (or max) required star version.

I don't think it is easy to add a new column in a table, this suppose to change the table (which happened when we add a new column for the 'genomeVersion')...

Yes, that would require a new table. I'm also not so sure whether that would improve the situation much.
min and max version checks in tool wrappers also need quite some discipline to maintain, and the max check in particular doesn't work backwards, i.e., at the time of writing a tool wrapper version the max value is typically unknown still so there's always at least one wrapper version that will display all newer index versions.
What would be comparably easy to do is to remove the symlink and have the DM use its own macro, which then needs to be maintained separately, but would maybe come with fewer surprises.

Anyway, I don't think this should hold back this PR any longer. If we want to decouple the DM from the tool wrapper, we should do it in its own PR where the decision will be more discoverable than as part of a giant PR like this one.

tools/rgrnastar/rg_rnaStar.xml

lldelisle · 2023-02-16T11:38:19Z

Let see if I did not break everything...

wm75

Thanks for the fixes, looks great to me now!

lldelisle · 2023-02-16T12:20:03Z

Youhou! Who wants to click or find something to change?

wm75 · 2023-02-17T08:14:45Z

Merging is ok with you @bernt-matthias ?

bernt-matthias · 2023-02-17T08:29:25Z

Sure. Thanks @lldelisle

lldelisle · 2023-02-17T08:30:15Z

🎉 this was a HUGE PR.

lldelisle · 2023-02-17T08:30:35Z

A great collaboration. I love you galaxy people.

lldelisle · 2023-02-17T08:31:28Z

This worth probably a blog post...

pavanvidem · 2023-02-17T09:05:23Z

Let see if I did not break everything...

The most important thing is that this was a huge progress. Thanks! I'm using this tool very frequently for 10x and Smart-seq data. I also have some iCell (needs the optional barcode whitelist param), cel-seq and large Smart-seq (needs limit params) data in the queue. I can already tell in the next weeks if we broke something ;-)

bgruening · 2023-02-17T10:43:23Z

This worth probably a blog post...

YEAH! 💯

bernt-matthias · 2023-02-17T18:06:51Z

Have we fixed all those issues #5060 (comment).. If so we can close them.

bernt-matthias · 2023-02-17T18:31:27Z

Hmm. Can't find the workflow run for the merge. Can someone double check?

bernt-matthias · 2023-02-17T18:31:47Z

At least the updates did not arrive at the TS yet.

bernt-matthias · 2023-02-17T18:33:38Z

Arg, the commit message contained [no ci] ...

bgruening · 2023-02-17T19:58:32Z

I take care. Thanks @bernt-matthias

lldelisle · 2023-02-22T13:31:13Z

Sorry I just found a bug see #5144

lldelisle · 2023-02-22T16:11:08Z

In fact 2 bugs... 😔 I PR a fix in #5145

lldelisle added 5 commits January 16, 2023 22:04

update STAR version

b52f63b

add GeneQuant + outSAMattributes

1006e84

remove dup test

f450b3e

update tool version [no ci]

a7cb191

lint

2b72bb8

mtekman reviewed Jan 17, 2023

View reviewed changes

tools/rgrnastar/rg_rnaStarSolo.xml Show resolved Hide resolved

mtekman approved these changes Jan 17, 2023

View reviewed changes

wm75 reviewed Jan 17, 2023

View reviewed changes

tools/rgrnastar/rg_rnaStarSolo.xml Outdated Show resolved Hide resolved

lldelisle added 2 commits January 18, 2023 07:45

add SAMattributes in macro as suggested by @wm75

8f42952

change Cell Ranger to Chromium chemistry

c3b1dc3

pavanvidem added 3 commits January 20, 2023 12:02

Update rg_rnaStarSolo.xml

45afe99

make `--soloCBwhitelist` optional and add limits macro for Smart-Seq

Update macros.xml

e91e9f3

Update rg_rnaStar.xml

662ba6d

bernt-matthias mentioned this pull request Jan 26, 2023

STAR: allow fasta.gz for reference #5101

Closed

5 tasks

lldelisle commented Jan 26, 2023

View reviewed changes

tools/rgrnastar/rg_rnaStar.xml Outdated Show resolved Hide resolved

STAR: allow fasta.gz for reference

033cd4a

plus: - removed a few redundant name attribs - increases profile

pavanvidem approved these changes Feb 15, 2023

View reviewed changes

wm75 reviewed Feb 16, 2023

View reviewed changes

add forgotten requirement

c6c4822

wm75 reviewed Feb 16, 2023

View reviewed changes

tools/rgrnastar/rg_rnaStar.xml Outdated Show resolved Hide resolved

use @TOOL_VERSION@+galaxy@VERSION_SUFFIX@

296660c

lldelisle force-pushed the starsolo_update branch from 5af8af0 to 296660c Compare February 16, 2023 11:31

lldelisle added 2 commits February 16, 2023 12:36

bump version of data_manager

53faaa4

put back to MAPQ60 with remark

e3966b9

wm75 approved these changes Feb 16, 2023

View reviewed changes

bernt-matthias merged commit ae6b59a into galaxyproject:main Feb 17, 2023

lldelisle deleted the starsolo_update branch February 17, 2023 08:30

		@@ -5,7 +5,8 @@
		the index versions in sync, but you should manually adjust the +galaxy
		version number. -->

Update STAR version + new options for STARsolo #5060

Update STAR version + new options for STARsolo #5060

Conversation

lldelisle commented Jan 16, 2023

lldelisle commented Jan 17, 2023

lldelisle commented Jan 17, 2023

mtekman left a comment

Choose a reason for hiding this comment

pavanvidem commented Jan 17, 2023 • edited Loading

lldelisle commented Jan 17, 2023

lldelisle commented Jan 17, 2023

pavanvidem commented Jan 17, 2023

pavanvidem commented Jan 17, 2023

lldelisle commented Jan 18, 2023

pavanvidem commented Jan 18, 2023

lldelisle commented Jan 18, 2023

bernt-matthias commented Jan 26, 2023

lldelisle commented Jan 26, 2023

pavanvidem commented Jan 26, 2023

bernt-matthias commented Jan 26, 2023

pavanvidem left a comment

Choose a reason for hiding this comment

lldelisle commented Feb 15, 2023

pavanvidem commented Feb 15, 2023

lldelisle commented Feb 15, 2023

wm75 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lldelisle commented Feb 16, 2023

wm75 left a comment

Choose a reason for hiding this comment

lldelisle commented Feb 16, 2023

wm75 commented Feb 17, 2023

bernt-matthias commented Feb 17, 2023

lldelisle commented Feb 17, 2023

lldelisle commented Feb 17, 2023

lldelisle commented Feb 17, 2023

pavanvidem commented Feb 17, 2023

bgruening commented Feb 17, 2023

bernt-matthias commented Feb 17, 2023

bernt-matthias commented Feb 17, 2023

bernt-matthias commented Feb 17, 2023

bernt-matthias commented Feb 17, 2023

bgruening commented Feb 17, 2023

lldelisle commented Feb 22, 2023

lldelisle commented Feb 22, 2023

pavanvidem commented Jan 17, 2023 •

edited

Loading