ronkeizer / vpc

R library to create visual predictive checks (VPC)
Other
36 stars 20 forks source link

Modularize Code Base #78

Closed billdenney closed 2 years ago

billdenney commented 3 years ago

This is a partial fix to #5. But, even with all these changes, it doesn't go all the ay to a complete modularization. The reason that it doesn't quite go all the way is that there are some ways that some parts interact (e.g. pred_corr, lloq, and uloq), and I think that the best way to separate out those interactions are more functional changes (making them all work at the same time, for instance).

If you search the code for "TODO", you can see the areas where I intentionally changed functionality (though by design, those changes were minimal, and they were mostly of the ilk of bug fixing rather than intentional changes). There are a few, relatively minor areas that are not called out like that (for instance quantile_cens() had some work that adds functionality, but it didn't change functionality that was there).

I tried to keep each commit focused on a single type of change, so you can either review point-by-point by reviewing each commit or you can review everything at once.

All tests that were present at the beginning still pass, and more tests have been added.

billdenney commented 3 years ago

Thank you!

Yes, I didn't like making it such a big PR, but as I started making some of the changes, they tended to overlap with each other in a way that didn't feel quite right to make separately. I do think that there will be another set of PRs to get everything into the right chunks, but I anticipate that the next ones will be more easily separable into easy-to-digest chunks.

I'll double-check that you caught all the places that needed specific review. I tried to tag everything that needed specific review with "TODO", so my check will be that you caught all of those.

And, you're correct that when large blocks moved, they were generally kept as-is. There may have been some minor code simplifications or other minor edits, but nothing major was changed in those.

billdenney commented 3 years ago

And, here are the additional review items that would still benefit from your eyes:

ronkeizer commented 3 years ago

And, here are the additional review items that would still benefit from your eyes:

* https://github.com/billdenney/vpc/blob/ccbc2474ba5e47cd48db84201338fd0749ab5a1f/R/calc_vpc_continuous.R#L26-L42

* https://github.com/billdenney/vpc/blob/ccbc2474ba5e47cd48db84201338fd0749ab5a1f/R/define_loq.R#L25-L26

* https://github.com/billdenney/vpc/blob/ccbc2474ba5e47cd48db84201338fd0749ab5a1f/R/filter_dv.R#L10-L14

* (not likely to be a big deal, but for completeness:) https://github.com/billdenney/vpc/blob/ccbc2474ba5e47cd48db84201338fd0749ab5a1f/R/format_vpc_input_data.R#L130-L131

* https://github.com/billdenney/vpc/blob/ccbc2474ba5e47cd48db84201338fd0749ab5a1f/R/format_vpc_input_data.R#L156-L161

* https://github.com/billdenney/vpc/blob/ccbc2474ba5e47cd48db84201338fd0749ab5a1f/R/plot_vpc.R#L226-L227

* https://github.com/billdenney/vpc/blob/ccbc2474ba5e47cd48db84201338fd0749ab5a1f/R/plot_vpc.R#L235-L237

* https://github.com/billdenney/vpc/blob/ccbc2474ba5e47cd48db84201338fd0749ab5a1f/R/plot_vpc.R#L345-L347

* https://github.com/billdenney/vpc/blob/ccbc2474ba5e47cd48db84201338fd0749ab5a1f/R/quantile_cens.R#L27-L29

* https://github.com/billdenney/vpc/blob/ccbc2474ba5e47cd48db84201338fd0749ab5a1f/R/show_default.R#L34-L35

* I'm not sure that I understood your comment above, so this TODO is still in the code: https://github.com/billdenney/vpc/blob/ccbc2474ba5e47cd48db84201338fd0749ab5a1f/R/vpc_tte.R#L154-L156

* https://github.com/billdenney/vpc/blob/ccbc2474ba5e47cd48db84201338fd0749ab5a1f/R/vpc_tte.R#L251-L253

confirmed, all fine with me.

billdenney commented 3 years ago

Alrighty, everything is now in.