Skip to content

Conversation

@ScottNortonPhD
Copy link

The option to test specific variants is documented in the Nature Protocols paper but is not implemented here. This PR contains a draft implementation of the variant detection workflow. More importantly, it fixes two crashes, one in each of the association and count workflows.

Copy link
Collaborator

@visze visze left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for this PR! There are some good catches and the highly demanded version option. Since this are multiple things (and maybe we need some additional work before release) can you make a PR to the development branch and have a look at my review comments here?

But thanks a lot again!

Comment on lines +45 to +61
process {
withLabel: longtime {
executor='slurm'
//queue='default'
clusterOptions = '-t 3-00:00:0 --mem=6G'
}
withLabel: shorttime {
executor='slurm'
//queue='default'
clusterOptions = '-t 00-01:00:0 --mem=6G'
}
withLabel: highmem {
executor='slurm'
queue='bigmem'
clusterOptions = '-t 00-20:00:0 --mem=80G'
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you revert this changes?
I think it is better to to leave the default without any cluster environment.
It is also documented like this in the docs

@@ -1,5 +1,5 @@
//MPRAflow version
params.version="2.3.4"
params.version="2.3.5"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think PR has multiple thinks (fixes and features) in it. So due to (semantic versioning)[https://semver.org/lang/de/] it is not a increase of the patch version.

Can you redirect the PR to the development branch? I think we have to do some steps (update documentation, fix some issues you found (hard coded BC length)) before creating a new release.

- tk=8.6.8=hbc83047_0
- wheel=0.33.6=py27_0
- xz=5.2.4=h14c3975_4
- zlib=1.2.11=h7b6447c_3
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason for this change?

Otherwiese I would leav eit as it is.

Comment on lines -6 to +8
- nextflow=20.01
- nextflow=20.10
- samtools
- conda<4.7
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason for teh NF increase and especially adding the other two software packages?

samtools should be handled by the environmen files I guess. Conda I don't know. do you have issues with it?

#print(counts)

mask=(counts['Barcode'].str.len() == 15)
mask=(counts['Barcode'].str.len() == 16) # Scott change - hardcoded barcode length is a bad idea, but so is spending a year refactoring this.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh that's crazy. thanks for the catch! But we should ass teh BC length via the options. In nexflow we knwo them so it should be no problem

res.insert(1, 'rna_count',(res.rna_sum+1) / (res.n_obs_bc+1) / rna_total * 10**6)

print(res_t)
#print(res_t)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would deleate the line.

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

Successfully merging this pull request may close these issues.

2 participants