plot pgs densities #24
Conversation
R/plot-pgs.R
Outdated
| density.multipanel <- BoutrosLab.plotting.general::create.multipanelplot( | ||
| plot.objects = pgs.density.plots, | ||
| filename = file.path(output.dir, filename.for.density.multiplot), | ||
| layout.height = 1, | ||
| layout.width = length(pgs.density.plots), | ||
| main = '', | ||
| main.cex = 0, | ||
| width = width, | ||
| height = height | ||
| ); | ||
|
|
||
| } | ||
| return(density.multipanel); # this returns null when filename is provided to create.multipanelplot |
There was a problem hiding this comment.
@dan-knight Pulling you in for a BPG question:
It seems that create.multipanelplot does not return a plot object if a filename is specified (it just writes the plot to the file). Is that expected behavior? Also, when an object is returned, it seems to be of class "multipanel" - is that something that was made up for BPG or is that an actual lattice-related class?
There was a problem hiding this comment.
Hmm. Off the top of my head, I don't think that should be the case. That would be inconsistent with the other plotting functions. It's possible that there's some technical limitation specific to multipanel plots, but I'd hope that wouldn't be the case. You can see here that it's returning the grob rather than returning the result of write.plot. This is inconsistent with other plotting functions..
The class name is set here. I think that should be changed to include the other trellis/grid classnames (adding multipanel) rather than setting it to only multipanel.
There was a problem hiding this comment.
@dan-knight would appreciate your thoughts on how my tests turned out. Is there a way to check dimensions of a multipanel object? I couldn't figure it out.
There was a problem hiding this comment.
Look good to me! I left another comment in the code, but the tests look good. I think with plotting code like this, you can accomplish a lot with validation as you have here. It's much easier to test, and lets you handle problems earlier on (rather than deep in the internal plotting code).
| density.multipanel <- BoutrosLab.plotting.general::create.multipanelplot( | ||
| plot.objects = pgs.density.plots, | ||
| filename = output.path, | ||
| layout.height = 1, |
There was a problem hiding this comment.
Only way you might test this is if you broke this logic out into another function and tested that. For example, you might write get.pgs.layout() which returns a width and height value. Then, you could test those values. This logic is sufficiently simple that it's probably not necessary unless you see the layout changing or becoming more complex in the future.
Add function for plotting density curves, optionally split by categorical phenotypes
Add tests for said function
I have read the code review guidelines and the code review best practice on GitHub check-list.
The name of the branch is meaningful and well formatted following the standards, using [AD_username (or 5 letters of AD if AD is too long)-[brief_description_of_branch].
I have set up or verified the branch protection rule following the github standards before opening this pull request.
I have added the changes included in this pull request to
NEWSunder the next release version or unreleased, and updated the date.I have updated the version number in
metadata.yamlandDESCRIPTION.Both
R CMD buildandR CMD checkrun successfully.Testing Results
All tests pass