Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Experimental support for GISTIC export #623

Merged
merged 3 commits into from
Jul 21, 2021

Conversation

tetedange13
Copy link
Contributor

@tetedange13 tetedange13 commented Jun 3, 2021

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
Copy link
Owner

etal commented Jun 4, 2021

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
Copy link
Contributor Author

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

  • A mandatory "seg" file, whose format matches output from cnvkit.py export seg <SEVERAL_CNS> (I mean GISTIC is taking it without error)
  • An optionnal "markers" file which is produced by cnvkit.py export gisitic <SEVERAL_CNR>

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
Copy link
Collaborator

tskir commented Jun 23, 2021

@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:

  • Adding some basic documentation about this format to the export section;
  • Since GISTIC presumably can only work with autosomes, the SEG export command should have a flag to only export autosomes, to avoid the mismatch which you encountered in your tests.

@tetedange13
Copy link
Contributor Author

tetedange13 commented Jun 24, 2021

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
Copy link
Collaborator

tskir commented Jul 1, 2021

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

  • Remove the .autosome() filter
  • Add minimal description of gistic export to the documentation and mark it as experimental

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

@tskir tskir marked this pull request as draft July 1, 2021 23:16
@tetedange13
Copy link
Contributor Author

Hi @tskir ,

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

Have a nice day.
Felix.

@tskir tskir marked this pull request as ready for review July 2, 2021 21:41
@tskir tskir requested review from tskir and etal July 2, 2021 21:41
Copy link
Collaborator

@tskir tskir left a comment

Choose a reason for hiding this comment

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

@tetedange13 Thank you Felix, you're a star!

@tskir tskir changed the title #622 Fix gistic export Experimental support for GISTIC export Jul 13, 2021
@tskir tskir added enhancement ready for merge Reviewed and approved at least once labels Jul 13, 2021
Copy link
Owner

@etal etal left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! The .autosomes() filter was there because I was under the mistaken impression that GISTIC only accepts autosomes as input.

@etal etal merged commit 3214cb6 into etal:master Jul 21, 2021
@tetedange13 tetedange13 deleted the 622-fix-gistic-export branch July 21, 2021 06:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement ready for merge Reviewed and approved at least once
Projects
None yet
Development

Successfully merging this pull request may close these issues.

How to export gistic markers?
3 participants