IQSS / Zelig

A statistical framework that serves as a common interface to a large range of models

http://zeligproject.org

111 stars 43 forks source link

Plot update #297

Open mbsabath opened 7 years ago

mbsabath commented 7 years ago

Added labels to the base Zelig graph axes

christophergandrud commented 7 years ago

Thanks @mbsabath

My main concern for this is ensuring that we've tested all cases and that they make sense. Maybe in a fork, could you compile all of the vignettes and that should implement the new labels for most plots. We will then need to manually check these.

christophergandrud commented 7 years ago

Also, from the CI check, it looks like the pull request doesn't pass CRAN check due to undefined global variables n.y1. You just need to start the offending function with n.y1 <- NULL

codecov-io commented 7 years ago

Codecov Report

:exclamation: No coverage uploaded for pull request base (master@b3cefb3). Click here to learn what that means. The diff coverage is 66%.

@@            Coverage Diff            @@
##             master     #297   +/-   ##
=========================================
  Coverage          ?   88.24%           
=========================================
  Files             ?       50           
  Lines             ?     4159           
  Branches          ?        0           
=========================================
  Hits              ?     3670           
  Misses            ?      489           
  Partials          ?        0

Impacted Files	Coverage Δ
R/model-timeseries.R	`87.65% <100%> (ø)`
R/plots.R	`77.96% <63.82%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update b3cefb3...2619baf. Read the comment docs.

mbsabath commented 7 years ago

The most recent commit took care of all the check notes on my machine

christophergandrud commented 7 years ago

Just looking over the plots created in the README for this pull request and thinking about ways forward.

In the following plot I like the "Density" y-axis label. That seems like a clear win. But in this plot the Predicted Values are labeled "Expected Values" on the x-axis. That should be changed.

In the next plot the bottom two figures have the x-axis label "Value", which isn't really informative.

Overall

The new y-axis labels seem good. But maybe the new x-axis labels are a bit inconsistent and potentially not informative (thus worsening the data-ink ratio). I think the issue is that in many cases the old plot titles (e.g. "First Differences: E(Y|X1) - E(Y|X)") describe the x-axis. So adding new x-axis labels is in most cases redundant.

I guess the question then is whether we should just move the current plot titles to be the x-axis labels?

mbsabath commented 7 years ago

I think that's a valid question. In my mind, there are two options for this. One of which is to change the X Label to be something basic like "Value" and leaving the title as it is, or in the event no xlabel is supplied, moving the default to be the label for the graph. Both of these solutions would be pretty easy to implement, and mostly come down to preference. I personally think that using the title as the default x label makes the most sense, but am not set on it. Do you have a preference?

christophergandrud commented 7 years ago

I'm adverse to the the value solution as this is non-informative. Perhaps moving the title to the x-axis sense. Lets ask James later today.

christophergandrud commented 7 years ago

Great. I'll take more of a look this weekend.

christophergandrud commented 7 years ago

Overall pretty nice. I noticed one oddity for Ordered Logistic Regression (and likely similar):

"Y Value" could really be "Outcome" as well.