Plot summary

[bcollica]

Updating pull request to merge plot_summary branch into master. Tests have been added, and descriptions have been updated. One final thing will be to add a 'Details' section in both the main plot and summary functions, but this can come later, along with examples.

[PhilBoileau]

I'm going to write out a few comments and questions as I go through the review. Some are targeted at specific sections of code, others are more general (listed here). No need to address these right away, we should meet to discuss them first.

• Given that cvCovEst returns tibbles, would it be better to have the functions relating to plotting and summary also work with tibbles instead of data.frames/data.tables?
• Some of the functions have hard-coded lists of estimators with/without hyperparameters. I realize that there probably isn't a straightforward way to provide this information to these functions, but this makes the plotting/summary function-related code more tedious to update when implementing new estimators. Are there alternatives?
• I've noticed a mix of styles when it comes to the length of tabs. Given that most of the package uses 2 spaces, we fix all the 4 space-tabs. I've been fixing this as I go through the code, but I've probably missed a few spots.
• Similarly, there's a mix of " and '. It's typically best practice to use ". I've tried to fix all quotes throughout, but probably missed a few. Sorry for being nitpicky!
• plot_fnuctions.R clocks in at almost 2,000 lines of code. I think we should consider dividing up its contents into more manageable files.
• Certain sections seem to repeat the exact same code multiple times (e.g. multiHyperRisk). Let's see if we can't reduce the amount of repetition so that maintenance is smoother.

[bcollica]

Hey all, these are the changes I've implemented so far:

• switch from data.frames to tibbles
• separating code into separate files
• change any remaining single quotes to double quotes
• add specific function references where needed (dplyr::)
• got rid of viridis and viridisLite - they actually weren't being explicitly used anymore since I'm using scale_color_viridis_d from ggplot.
• added Details section to plot and summary functions
• switched data.table::rbindlist() to dplyr::bind_rows()
• switched reshape2::melt() to base R function as.data.frame.table(). I looked into tidyr::pivot_longer(), but there doesn't seem to be a quick way to replicate exactly what melt() outputs, whereas as.data.frame.table() does exactly the same thing. Plus it's base R, so no extra imports.

Things not yet changed:

• using rlang functions to extract the function arguments of estimators in order to avoid hard coding has_hypers
• aes_string() to help reduce code in multiHyperRisk(). I looked into this a bit and tried implementing it in one section of the function without success, but it was only a quick attempt. Also the documentation on aes_string mentions that it is soft deprecated and suggests "using tidy evaluation idioms" instead. I do think the code can be cut down and streamlined using some tidy extraction, but I have to look closer and see what makes sense.
• adding examples to exported functions. I wanted to double check with you guys first on the best way of doing this. For any of our examples, we'll have to start by calling cvCovEst on some data similar to what we do for the tests. Do we have an idea of which data, real or simulated, we'd want to use?
• I also did not run styler::style_pkg() yet because the description was very daunting and mentions that you should inspect the results carefully after calling it lol. Maybe this will be the last thing we do before merging.

Let me know what you guys think about data for examples and anything else that comes to mind.

Brian

[PhilBoileau]

This is great Brian, thank you so much for all your work! If you're alright with it, I can take a stab at the first two remaining points on your to-do list this weekend. We can then meet this week to discuss the implementation of examples, and go over any remaining questions that you have. And that sounds like a good strategy regarding styler!

[bcollica]

Sounds good, Phil. The main issue with the multiHyperrisk function is passing arguments to filter, factorize, and arrange the data as needed depending on which estimator(s) are specified. If that information can be stored at a higher level in the function and then successfully passed down to the other functions like ggplot then the code can be much shorter. I know these tidyverse functions can be difficult to work around with their lazy evaluation, so if you have any thoughts I'm definitely open to suggestions.

[PhilBoileau]

I've taken a closer look at the plotting functions' code (but haven't touched anything), and I think that the first step in increasing its maintainability might be to wrap estimator-related code in standalone functions. These functions can then be called by the various summary and plotting functions as needed. We can then refactor these functions at some point in the future to take advantage of rlang functionality.

Here are two suggestions:

• In the multiHyperRisk function, you might consider moving the code inside the if-else statements to estimator-specific functions that are then called by multiHyperRisk. This organization will cut multiHyperRisk's length in half, and make it easier to extend with multi-parameter estimators implemented in the future. Perhaps these functions could be store in estimator-specific helper files?
• As discussed previously, plot.cvCovEst, multiHyperRisk, cvRiskPlot, cvSummaryPlot, and hyperRisk rely on some hard-coded estimator categorizations. I suggest writing an internal helper function that keeps track of the estimator characteristics, and then returns them as necessary. Maybe as a named list? These characteristics will obviously be hard-coded, but at least they'll only need to be hard-coded once in an easy-to-maintain function.

I'm sure that you can think of more examples, but these suggestions should be relatively painless to implement, and address the most challenging tasks remaining in this PR.

Let me know what you think. @nhejazi, your thoughts on this would also be much appreciated. I don't want to lead Brian down the wrong path.

Again, thank you for all your work on this!

[bcollica]

Just pushed those changes including separate plotting functions that are specific to estimators with multiple hyperparameters. Also included a estAttributes function to cut down hard coding.

[PhilBoileau]

This looks great! Thanks for making fast work of these requests.

[nhejazi]

LGTM! There's a few minor comments to address/discuss, but I'm good with merging after we've re-synced this with master and resolved those comments. This will be great functionality to have! I didn't see anything about it in the vignette, but we should eventually add a section there about this.