-
Notifications
You must be signed in to change notification settings - Fork 18
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
breakdown of components in vpc.R #5
Comments
you have a point. These vpc functions were clean at the start, but they have grown fast :) The general structure you list above is correct, and it is a good idea to extract the parts in single sub-function wherever possible. I think for the plot and "plot" and "customize" parts this should be relatively straightforward. Some parts of the pre_processing (e.g. checking/parsing arguments etc) can also be extracted as a general function, in fact this is already partially done in the function format_vpc_input_data(). However, the processing of data is more tricky, as it is fairly different between vpc for continuous, censored, categorical, and tte data. I propose to leave these functions alone for now and possibly come back to those later. We can start with the "plotting" and "customize" parts, as well as the "pre_processing". That would be a lot cleaner already. |
Sounds good - I'll start extracting those components - do you want me to make a branch or just stick directly in the master. My thought is for these 'reorganization' parts just doing in master should be fine, but when we attempt a conversion to OO probably will branch in case sh*t hits the fan trying to figure things out. |
Master is fine, changes won't be dramatic and the package is in development stage anyhow. |
After reading the code some it looks like we can 'break apart' these monolithic vpc functions and take a more modular approach to make maintaining and updating easier.
As it stands it looks like inside of vpc.R it handles:
The first thing I propose is we break apart the preproccessing, processing, 'base' plotting, and further customizations into individual functions, so the vpc() function would be little more than a wrapper along the lines of
given the overlap across vpc functions this would also help reduce copied code etc...
Thoughts?
The text was updated successfully, but these errors were encountered: