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

plot_multisample_CNA: code error in missing setting the blank_genome #12

Closed
qingjian1991 opened this issue Nov 1, 2021 · 4 comments
Closed
Assignees
Labels
bug Something isn't working

Comments

@qingjian1991
Copy link
Contributor

qingjian1991 commented Nov 1, 2021

Hi:

When use the plot_multisample_CNA() for plotting multi-regional CNAs, I found some errors in your codes

The original code is:
#line 62-66 #Default blank genome -- remove labels with label_chr = NA bl_genome = suppressMessages( CNAqc:::blank_genome(label_chr = NA) + labs(x = "", y = "") )

This code forgets to set some parameters. The corrected code may be:

bl_genome = suppressMessages(CNAqc:::blank_genome( ref = L[[1]]$reference_genome, chromosomes = chromosomes, label_chr = NA) + labs(x = "", y = ""))

In addition, I suggest adding some parameters in this function to control the output styles, like parameters of plot_segments as well as KARYO_colors and min_length_show ( the minimal length of chromosomes to show )

Yours sincerely,

Qingjian Chen

@caravagn
Copy link
Collaborator

caravagn commented Nov 3, 2021

Hi @qingjian1991

  • are you suggesting we miss the reference? If yes I think you are correct.
  • upgrades are indeed possible, only those 2 would be required in your opinion?

@caravagn caravagn self-assigned this Nov 3, 2021
@caravagn caravagn added the bug Something isn't working label Nov 3, 2021
@qingjian1991
Copy link
Contributor Author

Hi @caravagn

  • are you suggesting we miss the reference? If yes I think you are correct.

Reply: Yes, the original code missed the reference.

  • upgrades are indeed possible, only those 2 would be required in your opinion?

Reply: I only set the two parameters. In addition, users maybe want to show the total copy numbers, rather than Karyotypes. I think this set would be helpful for users.

Hi @qingjian1991

  • are you suggesting we miss the reference? If yes I think you are correct.
  • upgrades are indeed possible, only those 2 would be required in your opinion?

@caravagn
Copy link
Collaborator

Makes sense. Do add the options and open another PR for me to check, happy to accept if everything works.

@caravagn
Copy link
Collaborator

caravagn commented Sep 1, 2022

Thanks @qingjian1991

@caravagn caravagn closed this as completed Sep 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants