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

Diagram: switches for limiting by chromosome and for disabling gene labels #634

Merged
merged 4 commits into from
Jul 21, 2021

Conversation

tetedange13
Copy link
Contributor

@tetedange13 tetedange13 commented Jun 24, 2021

Enhancement on diagram regarding #629

Default:

211461558_S6-diagram2-1

With --no-gene-labels on:

211461558_S6-diagram3-1

Closes #628. Closes #629.

@tetedange13 tetedange13 changed the title Switch to disable gene_names from 'diagram' #629 Switch to disable gene_names from 'diagram' Jun 24, 2021
@tskir
Copy link
Collaborator

tskir commented Jul 1, 2021

Thanks Felix, everything looks good to me!

@tskir tskir added the plots label Jul 2, 2021
@tskir tskir requested a review from etal July 2, 2021 21:42
@tetedange13
Copy link
Contributor Author

tetedange13 commented Jul 6, 2021

Commit e8470dd should implement a feature discussed here
=> Display a single chromosome instead of all on diagram (exactly same display as in tskir's comment with awk-filtered ".cnr")
=> I implemented like scatter -c, except "start", "end" are ignored from unpacked range
=> Works also with --no-gene-labels

Kind regards.
Felix.

@tetedange13 tetedange13 requested a review from tskir July 6, 2021 10:08
@tskir tskir mentioned this pull request Jul 13, 2021
@tskir tskir changed the title Switch to disable gene_names from 'diagram' Diagram: switches for limiting by chromosome and for disabling gene labels Jul 13, 2021
cnvlib/diagram.py Outdated Show resolved Hide resolved
@tetedange13 tetedange13 requested a review from tskir July 15, 2021 07:11
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.

This code LGTM and I approve of the new feature. Without it, the way to get the same effect is by filtering the .cns and/or .cnr, as noted in the comments, to remove chromosomes or replace the gene names with -.

The metavar for -c might be better as "LABEL" or "NAME" ( however it appears in other commands) rather than "RANGE" to reduce the temptation to type a basepair-level range.

@tetedange13
Copy link
Contributor Author

Hi @etal,

Thanks for your code review !
=> I decided to not set metavar= parameter at all
=> This way this part from cnvkit.py diagram --help is pretty self-explanatory:

  -c CHROMOSOME, --chromosome CHROMOSOME
                        Chromosome to display, e.g. 'chr1' (no chromosomal range allowed)

Looking good to you?
Have a nice day.
Felix.

@tetedange13 tetedange13 requested a review from etal July 21, 2021 13:01
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.

All LGTM now

@etal etal merged commit a402d90 into etal:master Jul 21, 2021
@tetedange13 tetedange13 deleted the 629-turnoff-labels branch July 22, 2021 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

diagram parameter germline WES
3 participants