vincentarelbundock commented 5 years ago

Issue https://github.com/leeper/margins/issues/111

This is a big breaking change and still a work in progress, but I would like @leeper to see if the user interface is satisfactory.

I pasted some code below so you can just copy-paste and experience the main changes.

So far, the plot and cplot functions work for lm, glm and polr. If the approach is OK, I will extend to other models.

Internally, the main change is that instead of having different cplot methods, we have different cplot_extract methods, and cplot decides how to plot things depending on the standardized output of the extract methods. cplot_extract methods always return a data.frame with xvals and yvals columns, along with optional level, lower, upper columns. This allows some code re-use.

TODO list:

[x] rugs
[x] shades vs. lines = ribbon vs. 2x geom_line. Unless you care about this a lot, I'll mark this as "done" and implement geom_ribbon only.
[x] update docs param and examples
[x] more tests using the vdiffr testthat extension
[x] update README with some of the examples pasted below
[x] add table of contents to README for easier navigation
[x] bugfix in leeper/master prediction plots did not take into account the vcov argument at all.
[x] fix vignette compile
[x] polr nearly works. must disentangle x, dx, xvar
[x] plotting triple interactions (no more draw=add)

PR checklist:

[x] if suggesting code changes or improvements, open an issue first
[x] for all but trivial changes (e.g., typo fixes), add your name to DESCRIPTION
[x] for all but trivial changes (e.g., typo fixes), documentation your change in NEWS.md with a parenthetical reference to the issue number being addressed
[x] if changing documentation, edit files in /R not /man and run devtools::document() to update documentation
[x] add code or new test files to /tests for any new functionality or bug fix
[x] make sure R CMD check runs without error before submitting the PR

vincentarelbundock commented 5 years ago

@leeper Just to let you know, I think this is very close to being ready. The only big item remaining is to ensure that the polr models are working properly with stackedpredictions et al.

You might want to take a look at the updated README section on visualization here:

https://github.com/vincentarelbundock/margins/tree/base_to_ggplot#visualization

vincentarelbundock commented 5 years ago

The only major loss of functionality I'm aware of is the draw='add' for which I couldn't find a good solution in the current coding framework. But to me, that seems like a niche feature that expert users could easily do themselves by extracting the raw data with draw=FALSE.

Edit: maybe not. some people may care about triple interactions

Edit2: This can now be done with the z and zvals parameters. Each marginal effect is drawn in a different facet for now. https://github.com/leeper/margins/pull/114/commits/5a46285fd75a608f28fe561e682f61e1c3343512

codecov-io commented 5 years ago

Codecov Report

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

@@            Coverage Diff            @@
##             master     #114   +/-   ##
=========================================
  Coverage          ?   72.77%           
=========================================
  Files             ?       40           
  Lines             ?     1157           
  Branches          ?        0           
=========================================
  Hits              ?      842           
  Misses            ?      315           
  Partials          ?        0

Impacted Files	Coverage Δ
R/cplot_utilities.R	`0% <ø> (ø)`
R/cplot.R	`0% <0%> (ø)`
R/cplot_extract.R	`0% <0%> (ø)`
R/cplot_clm.R	`0% <0%> (ø)`
R/cplot_polr.R	`0% <0%> (ø)`
R/plot.R	`0% <0%> (ø)`

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 548943c...019e979. Read the comment docs.

vincentarelbundock commented 5 years ago

@leeper

Sorry this took so long.

I think this PR is now ready to merge. Travis shows no warn/error/note in build check, except a note related to a missing GhostScript executable (for pdf size check).

Of course, I am more than happy to make any changes you think are necessary. I also plan to stick around long term if tweaks or fixes are needed in the plotting files.

vincentarelbundock commented 4 years ago

I would have some time in the coming weeks to make changes to this PR, if needed. Let me know if there's anything I can do to make reviewing easier.

Obviously, I know that reviewing can be a major time commitment, and that you may prefer to not merge at all. That wouldn't be a major issue for me; just let me know if that's the case so I can close the PR.

vincentarelbundock commented 4 years ago

@leeper , I'm closing this PR for now.

I need to use the ggplot2 output for a few papers, and it feels cumbersome to install a special branch of margins every time I need this. So I'm spinning the PR off as a separate package:

https://github.com/vincentarelbundock/marginsplot

Obviously, I would much prefer if this were (eventually) integrated into margins directly. So please let me know if/when you'd like me to make a new, clean PR (tests and checks are all working right now).

leeper / margins

WIP: base -> ggplot2 #114

Codecov Report