mcguinlu / robvis

A package to quickly visualise risk-of-bias assessment results
https://mcguinlu.github.io/robvis/
Other
58 stars 22 forks source link

Generic language traffic lights #91

Closed AJFOWLER closed 4 years ago

AJFOWLER commented 4 years ago

PR Checklist

All Submissions:

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

Introduces customisable labels for the x axis, y axis, judgements and judgement title in the generic template.

What is the current behavior? (You can also link to an open issue here)

Current behaviour is hard coded options for the four parameters mentioned above.

What is the new behavior (if this is a feature change)?

New parameters passed to rob_traffic_light function to allow these features on the plot to be customised.

Additional change of creating judgement_levels variable to assign correct names to the judgement_labels

Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)

No

Other relevant information:

Two questions:

  1. Should users be able to enter differing levels of judgement, rather than just conforming to the standard - this would require substantial changes.

  2. Is the method of adding additional, optional parameters in keeping with the preferred style? Happy to adapt if not.

Thank you for taking the time to submit this PR!

82

AJFOWLER commented 4 years ago

It looks like this has failed because of a warning related to mismatched documentation @mcguinlu

I have added the @param arguments to the .R files, but didn't edit the .Rd files per the 'contributing' document.

I didn't re-build documentation while I was making the pull request because I wasn't sure if this constituted 'editing .Rd files' !

Do you want me to re-make documents in my branch and then re-issue PR?

mcguinlu commented 4 years ago

Hi @AJFOWLER,

Thanks for this (and for your patience while I've been on annual leave for the last few weeks)! Your changes look good - thanks for fixing the failing build. It's passing now.

Just from a user-focused point of view, I am worried that because the new arguments are available for all templates, users will assume they can edit the title and other aspects across the board. For example, the following seems acceptable per the documentation, but will throw an error:

rob_traffic_light(data_rob2, "ROB2",xtitle = "Test")

I'm pretty sure I know a way around this, by keeping the general rob_traffic_light() function, but using it as a way to point towards specific sub-functions for each template, e.g.:

rob_traffic_light <- function(data, tool, ...) {

if(tool == "ROB2"){
rob_traffic_light_rob2(data, ...)
}

if(tool == "Generic"){
rob_traffic_light_generic(data, ...) # This will just contain the code you have written
}

}

where ... in the general function is used to capture the arguments specify to that template and pass them to the template specific function. This fits in with a broader plan to add extra functionality for specific templates, while also maintaining backward compatibility. However, this will involve me doing some refactoring of the function to split out each template into its own internal function.

So I have pulled your changes into the development branch (rob_v_1) where I plan to make these wider changes (ideally, I hope to have them done within the next few weeks). Once I merge this branch into themaster`, you'll appear as a contributor to the project.

Thanks again for adding this code, and let me know if you have any major thoughts on my proposed plan!

AJFOWLER commented 4 years ago

Thanks Luke, hope you got a good rest on annual leave!

I now understand what you meant in your initial issue comment, sorry for my misunderstanding! I think it seems a sensible approach. I imagine there will be a base template +/- some helper functions where common functionality could be reused (e.g. cleaning up data).

Do you want me to do some work refactoring those templates or code functionality out from the base rob_traffic_light function on rob_v_1?

mcguinlu commented 4 years ago

Yes, got a good break thank you!

Not at all - in all honesty, your offer of help has just motivated me to finally implement the restructure which has been on the horizon for a while.

Just to check that my proposed work-around would in fact work, I split out the rob_traffic_light() function into its component parts yesterday - these changes have now been pulled into the master branch. If you are interested, a similar refactoring will need to be done for the rob_summary() function (and if you have any improvements/suggestions on the bare minimum I implemented in rob_traffic_light(), feel free to implement them). In addition, as you suggest, feel free to split out common functionality/reused code into its own function - I've done this already for the colour-defining step, but there is still a tonne of code that is repeated between each template (e.g. data cleaning, defining basic ggplot elements, etc).

Also, if you are interested, there is a load of new functionality/streamlining that the new approach allows for, as template-specific functionality will now be a lot easier to implement. Examples include an option to have paired risk of bias/applicability plots for the QUADAS-2 tool (implemented with patchwork), the removal of the ROB2-Cluster template in favour of an cluster argument that is passed to the general rob_traffic_light(data_rob2, "ROB2", cluster = TRUE) function call, and more. Let me know if anything here piques your interest, and I can write up more detailed description. Alternatively, you have any suggestions on further functionality you think would be useful, just let me know.

In essence, I'm very glad for the help, and anything you fancy tackling is yours!