etal / cnvkit

Copy number variant detection from targeted DNA sequencing
http://cnvkit.readthedocs.org
Other
540 stars 164 forks source link

Experimental support for GISTIC export #623

Closed tetedange13 closed 3 years ago

tetedange13 commented 3 years ago

Should fix issue #622 => Simply copy-pasted (+ adapted) code for 'jtv' export (same case with multiple ".cnr" as input) to enable 'gistic' export through command-line => This supposes "core" function for "gistic" export (within cnvlib/export.py) is correct, which I have not test => Maybe further tests are required ?

Closes #622

etal commented 3 years ago

Looks good, thanks. I think I left this feature as hidden because I hadn't tested its results with Gistic myself. Are you in a position to check whether the output of this command works with Gistic without further modification?

tetedange13 commented 3 years ago

Hi @etal ,

Sorry, I have never used GISTIC and I am completely unfamiliar with it => However I installed it and as far as I understood it takes 2 input files that can be both produced by cnvkit.py export

Feeding GISTIC only with mandatory "seg" file works fine, but I got an issue feeding it with both "seg" and "markers" files:

GISTIC 2.0 input error detected:
74 segment start or end positions in 'cns.gistic.seg' do not match any markers in 'cnr.gistic.markers'.
First bad position is X:150501 at line 114.

=> I guess it is because cnvkit.py export gistic works only on autosomes (contrary to cnvkit.py export seg) => Fortunately, filtering out sexual chromosomes from "seg" file produced by CNVkit is enough for GISTIC to finish without error !

To conclude: this seems OK to me, but as I said I do not know enough about GISTIC to assert results generated from "GISTIC + CNVkit's exports" are accurate => Maybe you should rather wait for BioComSoftware feedback ?

Hope this helps. Have a nice day. Felix !

tskir commented 3 years ago

@tetedange13 Thank you for the PR Felix! I agree we should ideally wait for @BioComSoftware's feedback prior to merging this.

I also think there are two additional things to be done here:

tetedange13 commented 3 years ago

Hi @tskir,

Since GISTIC presumably can only work with autosomes

Just to be clear, GISTIC itself can deal with sex-chrom (even if they are removed by default according to doc and that I could not manage to make it keep them with -rx=0 param...) => The issue I was getting comes from inconsistency between cnvkit.py export seg that includes sex-chrom and current cnvkit.py export gistic that removes them internally (based on here) => I do not know if there was a specific reason for that, at the time export_gistic_markers() was written?

To sum up: => If you want I can commit a sex-chrom switch for cnvkit.py export seg => But would not it be better to simply remove the .autosome() filter from export_gistic_markers() ? (I tested, GISTIC is taking both files just fine)


Kind regards. Felix.

tskir commented 3 years ago

Based on the feedback we received, I think this would be the most reasonable set of changes:

@tetedange13 Do you think you could add those changes to this PR?

tetedange13 commented 3 years ago

Hi @tskir ,

Done ! To document GISTIC export I did my best based on code docstrings + GISTIC documentation

Have a nice day. Felix.