Fancy Plot Updates

[mattssca]

This PR streamlines a collection of parameters throughout the fancy plot functions. It also replaces internal calls of assign_cn_to_ssm with the more appropriate function get_ssm_by_sample (issue #18 ). Lastly, the examples are updated to now run and the documentation had a substantial update in the out-of-date sections.

[vladimirsouza]

copy_number_vaf_plot also uses assign_cn_to_ssm internally (here). Should this function be included in this PR as well?

[mattssca]

This plot (copy_number_vaf_plot) is actually using the specific return from assign_cn_to_ssm, so for this plot to function correctly with just_segments = FALSE, assign_cn_to_ssm needs to be called.

[vladimirsouza]

There is an error in comp_report.

> comp_report(this_sample = "HTMCP-01-06-00422-01A-01D",
+             out = "./",
+             export_individual_plots = TRUE)
Using the bundled SSM calls (.maf) calls in GAMBLR.data...
 Error:  You have given one or more unsupported or deprecated argument to  get_ssm_by_sample . Please check the documentation and spelling of your arguments.
Offending argument(s): this_sample_id 

get_ssm_by_sample uses these_sample_ids instead of this_sample_id.

> args(get_ssm_by_sample)
function (these_sample_ids = NULL, these_samples_metadata = NULL, 
    this_seq_type = "genome", projection = "grch37", these_genes, 
    min_read_support = 3, basic_columns = TRUE, maf_cols = NULL, 
    verbose = FALSE, ...) 

[mattssca]

Right, this will have to be addressed. See this issue for more info.

[mattssca]

I have updated the internal call of get_ssm_by_sample to work with comp_report in GAMBLR.data for GAMBLR.results compatibility, see the issue I linked in my previous comment.

[vladimirsouza]

I think it makes more sense get_ssm_by_sample to use this_sample_id (both singular) and get_ssm_by_samples to use these_sample_ids (both plural). GAMBLR.results is already like this. For compatibility, GAMBLR.data should be the same. So GAMBLR.data::get_ssm_by_sample using these_sample_ids would be a problem and I think the last commit could be reverted.

I created a new PR in GAMBLR.data that changes get_ssm_by_sample to use this_sample_id.

[mattssca]

If we want to stick with this suggestion (having get_ssm_by_sample call this_sample_id parameter) there are numerous instances where this parameter needs to be updated in this PR (internal calls of get_ssm_by_sample(these_sample_ids)). I can update them and notify you when it's ready for re-review. Let me first review the PR you mentioned and then I can run some tests and update this PR accordingly.

[vladimirsouza]

That is great! Thanks @mattssca!

[vladimirsouza]

Just for the record, I don't think comp_report and fancy_sv_sizedens will work with the bundled data. This is due to the lack of variants (SSMs, mainly SVs). If using GAMBLR.results, they are fine.

[mattssca]

Yes, we discussed this limitation in Slack last week, when I raised the question if we could bundle more SVs (additional SV types) in the GAMBLR.data::sample_data. Currently, many of the SV plots are not very meaningful when using the bundled data. But thanks for noticing when testing as well!

[mattssca]

@vladimirsouza can you approve this PR, if all your concerns have been resolved? I think I have addressed them all.