carloscinelli / sensemakr

Suite of sensitivity analysis tools for OLS
https://carloscinelli.com/sensemakr/
88 stars 16 forks source link

Accessing worstcaseplot and contourplot #14

Closed chadhazlett closed 7 years ago

chadhazlett commented 7 years ago

We had envisioned these being either called via plot.sensemade(type=), or directly by the user. The latter is not generally working -- special quantities are computed in plot.sensemade, such as benchmarks$benchmarks_2plot1, which are not present if user calls directly. For now I'm removing the "@export" on these, but we should decide if we want them to be user facing, and clean up as needed if we do.

statsccpr commented 7 years ago

I see, that's exactly right.

The topmost generic plot.sensemade has the portion that takes

the showvars arguments into subsets quantities assigns into

benchmarks$benchmarks_2plot1 benchmarks$benchmarks_2plot2

then dispatches to contourplot() worstcaseplot()

contourplot and worstcase plot will then only display

benchmarks$benchmarks_2plot1 benchmarks$benchmarks_2plot2

depending on the decision moving forward we can tuck in this showvars assignment step into these two specific methods if desired

chadhazlett commented 7 years ago

Thanks Michael. I'm open to what you guys think on this. At the moment it would seem expeditious to require the user to go through plot().

On Mon, Aug 14, 2017 at 1:53 PM, statsccpr notifications@github.com wrote:

I see, that's exactly right.

The topmost generic plot.sensemade has the portion that takes

the showvars arguments into subsets quantities assigns into

benchmarks$benchmarks_2plot1 benchmarks$benchmarks_2plot2

then dispatches to contourplot() worstcaseplot()

contourplot and worstcase plot will then only display

benchmarks$benchmarks_2plot1 benchmarks$benchmarks_2plot2

depending on the decision moving forward we can tuck in this showvars assignment step into these two specific methods if desired

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/chadhazlett/sensemakr/issues/14#issuecomment-322306612, or mute the thread https://github.com/notifications/unsubscribe-auth/AFVCyCiPfIJdgMv3B9l-w0WI87chCdArks5sYLPUgaJpZM4O1SoF .

statsccpr commented 7 years ago

While adding examples and parameter documentations to the generic plot() and nonexported worstcaseplot() contourplot(), convinced me that we should do one or the other.

  1. Only rely on the generic and do not export worstcase + contour or
  2. get rid of the generic entirely, and rely on worstcase() + contourplot()

I think the generic plot() in concept, is nice since it forces the default plot to be the contour plot. But that's about it. the generic plot() doesn't save typing at all, if you want to get the worstcase option plot(worstcase) is actually more than the direct worstcaseplot()

So, I'm actually thinking the generic plot() is not really worthwhile. Especially since there are custom arguments that do not really align with the vanilla arguments in plot(),

Long story short, I think I'm leaning towards casting my vote to abandoning generic plot() while exporting and relying worstcaseplot() and contourplot()

I can lean the other direction too, but definately do not think we should support both

A negative about using generic s3 plot method is that looking up documentation is hard for someone not knowing where to look

the user needs to know to look at ?plot.sensemakr

since ?plot is not helpful

This would be in support of exporting ?countourplot or ?worstcaseplot since they can can have their own distinguished help docs

thoughts?

statsccpr commented 7 years ago

to try out both options, both of the lower level plots are directly accessible now, can also be exported

https://github.com/chadhazlett/sensemakr/commit/51422957188f5e512be1f92f1726cf9ad4708baa

So currently, can use all 3: generic, worstcase, contour

statsccpr commented 7 years ago

Closing issue for now, all 3 work, generic plot() + worstcase() + contour() all 3 share the same single documentation ?plot.sensemakr ?worstcaseplot ?contourplot

can revisit if we do not want all 3 available