Feature ptsize

[jcvdav]

This PR is based on #26. It provides support for a pt.size argument that controls the size of markets when geom_style = "pointrange" is used. In the absence of an argument, pt.size defaults to the values set in GeomPoint$default_aes$size. Checks are handled by ggplot2 internals.

The dev branch seems to be inactive, so suggesting this be merged to main instead.

library(fixest)
library(ggiplot)
#> Loading required package: ggplot2

# These examples borrow from the fixest::iplot() documentation and the
# introductory package vignette.

#
## Example 1: Vanilla TWFE
#

data(base_did)
base_inter = base_did

est_did = feols(y ~ x1 + i(period, treat, 5) | id+period, base_inter)
# Default behaviour
ggiplot(est_did)

# Controling for pt.size
ggiplot(est_did, pt.size = 10)

^{Created on 2023-10-27 with reprex v2.0.2}

[grantmcdermott]

Hmmm. Looks like quite a few tests are failing.

I'm out right now, but can you test locally with tinytest::test_all()?

The snapshots are only run on Ubuntu, so you'll need that distro. See here: https://github.com/vincentarelbundock/tinysnapshot

[jcvdav]

Fascinating... It passed all tests and checks locally. I'll do more digging to figure out what's going on on Monday.

[jcvdav]

Looks like the tests on iplot_data are the ones that are not passing, and it's all related to how CIs are calcuated in different versions of fixest:

Using the CRAN release (V0.11.0) I get the following:

> tinytest::test_all()
test_fixest_multi.R...........    0 tests    33ms [Exited at #5: Linux snapshots]
test_ggiplot.R................    0 tests    28ms [Exited at #5: Linux snapshots]
test_iplot_data.R.............   22 tests 4 fails 38ms
----- FAILED[data]: test_iplot_data.R<69--73>
 call| expect_equivalent(iplot_data_est[[col]], iplot_data_est_known[[col]], 
 call| -->    tolerance = tol)
 diff| Mean relative difference: 0.009186607
 ----- FAILED[data]: test_iplot_data.R<69--73>
 call| expect_equivalent(iplot_data_est_log[[col]], iplot_data_est_log_known[[col]], 
 call| -->    tolerance = tol)
 diff| Mean relative difference: 0.01716456
 ----- FAILED[data]: test_iplot_data.R<69--73>
 call| expect_equivalent(iplot_data_est[[col]], iplot_data_est_known[[col]], 
 call| -->    tolerance = tol)
 diff| Mean relative difference: 0.006470814
 FAILED[data]: test_iplot_data.R<69--73> expect_equivalent(iplot_data_est_log[[col]], iplot_data_est_log_known[[col]], ...
 
Showing 4 out of 22 results: 4 fails, 18 passes (0.1s)

Further inspecting the test script confirms that tests fail for col = "ci_low" or "ci_high". This seems to be related to this PR, since the ci values that are hard-coded into the test here seem to be retrieved using a t-distribution.

After installing the stable development version of fixest, the tinytests produce the following:

> tinytest::test_all()
test_fixest_multi.R...........    0 tests    0.1s [Exited at #5: Linux snapshots]
test_ggiplot.R................    0 tests    26ms [Exited at #5: Linux snapshots]
Loading required package: ggplot2
test_iplot_data.R.............   22 tests OK 0.7s
All ok, 22 results (0.8s)

Thoughts?

[grantmcdermott]

Ah, sorry I should have anticipated this. The dev version of fixest includes a dof correction to the iplot conf. interval calculation. lrberge/fixest#408

The ggiplot test snapshots are built against this corrected dev version.

What I don't get, though, is why the GH Actions CI test suite for this PR isn't pulling in the latest dev version. I've specified the necessary remote in the DESCRIPTION file. I'll try to take a peak later today.

[jcvdav]

Your DESCRIPTION file is correctly formatted and the R-CMD-check is installing the correct version in L 3898 here shows that fixest 0.11.1 2023-10-27 [1] Github (lrberge/fixest@a4d1a9b) is installed. This is the 6th commit following your PR to fixest, and I see some other things were modified following that.

[grantmcdermott]

Hey JC, sorry for the delay on this. I've been working on support for (gg)coefplot, which has required a fairly major code refactoring...

In the meantime, the new release of fixest has now reached CRAN. I have this change reflected in my coefplot branch, but for your purposes it might just be simplest to include them directly in this PR. We need only changed the DESCRIPTION file to require fixest >= 0.11.2 and can then drop the Remotes entry. Hopefully all the tests run properly once this update is completed.

[jcvdav]

I see you made the changes to the DESCRIPTION file, thanks for doing that. Unfortunately, it looks like some tests are still failing even when fixest 0.11.2 is installed. Thoughts?

[grantmcdermott]

Finally managed to test this locally and I can see what's causing them to fail now...

Background: Instead of using geom_pointrange (etc.) directly in the plot construction, I manually create the same effect by first running the desired interval style (e.g., geom_linerange for CIs without heads), followed by geom_point to get the point estimate. This produces the same effect as (say) a regular pointrange, but in a way that makes it easier to re-use other parts of the code internally (e.g., if a user asks for error bars or ribbons instead). HOWEVER... it does require some manual adjustment to the points themselves, since geom_pointrange & co. have a larger point size than vanilla geom_point.

The upshot is that using GeomPointGeomPoint$default_aes$size to get the "natural" point size is going to be too small.

EDIT/ADD: A simple solution to pass the test is obviously just to re-hardcode the pt.size = 2.5 default value again. You'd still be able to change this on the fly, thanks to your new "pt.size" argument. But it does mean that the ggplot2::update_geom_defaults path won't work. This seems like an acceptable tradeoff from my perspective, but this is your PR so I'll let you make the final call.

(Maybe there's a smarter way involving GeomPointrange defaults. But whatever option you decide, I do want to keep the larger point size as default.)

[jcvdav]

I think defaulting to pt.size = 2.5 is appropriate. I had also modified the DESCRIPTION file locally, hence the required merge. It should all be working now (?).

[grantmcdermott]

Checks passed 🎉