search

JMSLab / eventstudyr

Other
24 stars 1 forks source link

Release to CRAN #28

Closed jmshapir closed 1 year ago

jmshapir commented 1 year ago

@nateschor are you up for taking the lead on releasing to CRAN?

Our to-do list for this is here.

Steps proposed by R Packages

Ordered steps

1. Determine the release type, which dictates the version number. 2. If the package is already on CRAN: Do due diligence on existing CRAN results. If this is a first release: confirm you are in compliance with CRAN policies. 3. Freshen up documentation files, such as README.md and NEWS.md. 4. Double check() that your package is passing cleanly on multiple operating systems and on the released and development version of R. 5. Perform reverse dependency checks, if other packages depend on yours. 6. Submit the package to CRAN and wait for acceptance. 7. Create a GitHub release and prepare for the next version by incrementing the version number. 8. Publicize the new version.

Suggested checklist for initial CRAN submission

Checklist

- [x] ~~[usethis::use_news_md()](https://usethis.r-lib.org/reference/use_news_md.html)~~ - [x] [usethis::use_cran_comments()](https://usethis.r-lib.org/reference/use_cran_comments.html) - [x] Update (aspirational) install instructions in README - [x] Proofread Title: and Description: - [x] Check that all exported functions have @returns and @examples - [x] Check that Authors@R: includes a copyright holder (role ‘cph’) - [x] Check licensing of included files - [x] Review https://github.com/DavisVaughan/extrachecks - [x] Use `devtools::submit_cran()` to submit to CRAN

Tasks

Must

- [x] Go through the [CRAN Repository Policy](https://cran.r-project.org/web/packages/policies.html#Preamble) - [x] Run `devtools::load_all(".")` followed by `devtools::install()` described [here](https://r-pkgs.org/whole-game.html#install) and address any messages - [x] Check the [.Rbuildignore](https://r-pkgs.org/structure.html#sec-rbuildignore) file to make sure that correct files are ignored - [x] Confirm that we are shipping the vignette correctly, see https://github.com/JMSLab/eventstudyr/issues/28#issuecomment-1487667540 - [x] Address all errors, warnings, and notes in `devtools::check()` - [x] Address all errors, warnings, and notes in `devtools::document()` - [x] Update license year from 2022 to 2023 [here](https://github.com/JMSLab/eventstudyr/blob/fc81872e2e1f3fb17ae3d7fa108fb6a029f8d02f/LICENSE.md) and [here](https://github.com/JMSLab/eventstudyr/blob/fc81872e2e1f3fb17ae3d7fa108fb6a029f8d02f/LICENSE) - [x] Include `AddSuptBand` in the [Authors@R](https://r-pkgs.org/license.html#sec-how-to-include) section [here](https://github.com/JMSLab/eventstudyr/blob/fc81872e2e1f3fb17ae3d7fa108fb6a029f8d02f/DESCRIPTION#L4-L34) - [x] Include a [LICENSE.note](https://r-pkgs.org/license.html#sec-how-to-include) - [x] [Remove periods](https://r-pkgs.org/man.html#title-description-details) from the end of package titles in documentation, see [EventStudyPlot](https://github.com/JMSLab/eventstudyr/blob/fc81872e2e1f3fb17ae3d7fa108fb6a029f8d02f/R/EventStudyPlot.R#L1) for a function that ends in a period - [x] Specify [return value](https://r-pkgs.org/man.html#sec-man-returns) for `TestLinear`, `PrepareLeads`, and `PrepareLags` since user facing functions must document their return value - [x] Make sure that we have [VignetteBuilder](https://r-pkgs.org/vignettes.html#sec-vignettes-workflow-writing) in `DESCRIPTION` - [x] Mention in `cran-comments.md` that this is our first submission, described in the last bullet point [here](https://r-pkgs.org/release.html#double-r-cmd-checking) with an example of a comment file [here](https://r-pkgs.org/release.html#sec-release-cran-comments) - [x] Check [reverse dependencies](https://r-pkgs.org/release.html#sec-release-revdep-checks) (we shouldn't have any, I ran `devtools::revdep("eventstudyr")` and got `character(0)`. If I run `devtools::revdep("estimatr")`, for example, we get a number of packages) - [x] Figure out where to put [DEVELOPMENT.MD](https://github.com/JMSLab/eventstudyr/blob/fc81872e2e1f3fb17ae3d7fa108fb6a029f8d02f/DEVELOPMENT.MD)--since it's not a standard file in an R package repo, I don't think CRAN will like it being a top level file - [x] Based on `set.seed()` being listed as a function that should be used with caution [here](https://r-pkgs.org/code.html#sec-code-r-landscape), think about if we need to adjust how we set the seed in [AddSuptBand](https://github.com/JMSLab/eventstudyr/blob/fc81872e2e1f3fb17ae3d7fa108fb6a029f8d02f/R/AddSuptBand.R#L69)

Should

- [x] Think again about if we should still be making `PrepareLeads`, `PrepageLags`, and `TestLinear` [user facing](https://github.com/JMSLab/eventstudyr/issues/17#issuecomment-1448268132) * Looking at [this](https://r-pkgs.org/dependencies-in-practice.html#what-to-export) section, the [slider](https://www.tidyverse.org/blog/2020/02/slider-0-1-0/) package and [dplyr's](https://dplyr.tidyverse.org/reference/lead-lag.html) `lead` and `lag` function can do what `PrepareLeads` and `PrepareLags` do - [x] Set up package [citation](https://r-pkgs.org/misc.html#sec-misc-inst-citation) - [x] Add link to the `eventstudyr` repo in `DESCRIPTION`, we can also add a link that is helpful for [bug reportting](https://r-pkgs.org/misc.html#sec-misc-inst-citation) - [x] ~~Think about if we want to require a [minimum version](https://r-pkgs.org/description.html#sec-description-imports-suggests-minium-version) for the R packages we [use](https://github.com/JMSLab/eventstudyr/blob/fc81872e2e1f3fb17ae3d7fa108fb6a029f8d02f/DESCRIPTION#L38-L48), #14 is related~~ - [x] Think about if we want to change importing the [entire package](https://github.com/JMSLab/eventstudyr/blob/fc81872e2e1f3fb17ae3d7fa108fb6a029f8d02f/NAMESPACE#L8-L14) to only [specific functions](https://github.com/JMSLab/eventstudyr/blob/fc81872e2e1f3fb17ae3d7fa108fb6a029f8d02f/NAMESPACE#L15-L22) from a package since fully importing more entire packages increases the chance of [function namespace conflicts](https://r-pkgs.org/dependencies-in-practice.html#in-code-below-r) (final paragraph in linked subsection) - [x] Potentially add a [description](https://r-pkgs.org/man.html#description) to our user facing functions, since we currently don't have one and the function title gets copied to description (giving the same exact information twice)

Would Like

- [x] ~~Run `devtools::dev_sitrep()` [described here](https://r-pkgs.org/setup.html#verify-system-prep) and update everything that is out of date~~ - [x] ~~Based on [this](https://mikemcquaid.com/robot-pedantry-human-empathy/), move the [raw Stata files](https://github.com/JMSLab/eventstudyr/tree/fc81872e2e1f3fb17ae3d7fa108fb6a029f8d02f/examples/source/raw/eventstudy_illustration_data/orig) and [README](https://github.com/JMSLab/eventstudyr/blob/fc81872e2e1f3fb17ae3d7fa108fb6a029f8d02f/examples/source/raw/eventstudy_illustration_data/README.md) to a new folder called `inst/extdata/` (making sure to avoid any of the names listed [here](https://r-pkgs.org/misc.html#sec-misc-inst))~~ - [x] ~~In function documentation, switch to [inheriting arguments](https://r-pkgs.org/man.html#inheriting-arguments) so that we write function argument documentation once and use that same language each time that argument is used, for example, `timevar` [here](https://github.com/JMSLab/eventstudyr/blob/fc81872e2e1f3fb17ae3d7fa108fb6a029f8d02f/R/EventStudy.R#L8) and [here](https://github.com/JMSLab/eventstudyr/blob/fc81872e2e1f3fb17ae3d7fa108fb6a029f8d02f/R/PrepareLeads.R#L5)~~ - [x] ~~Potentially [merge](https://r-pkgs.org/man.html#sec-man-multiple-functions) the documentation for `PrepareLeads` and `PrepareLags` since they're so similar~~ - [x] ~~Potentially add `usethis::use_cran_badge` to show on our `README` the current version of our package on [CRAN](https://r-pkgs.org/other-markdown.html#readme.rmd-and-readme.md)~~ - [x] ~~Pair our version releases with a [NEWS.md](https://r-pkgs.org/other-markdown.html#sec-news) file, see [dplyr](https://dplyr.tidyverse.org/news/) example~~ - [x] ~~Create a [hex sticker](https://github.com/GuangchuangYu/hexSticker) for our package that shows a plot created by `EventStudyPlot` (looks quick and straightforward to do), see https://github.com/JMSLab/eventstudyr/issues/28#issuecomment-1482979547~~ - [x] ~~Potentially use [use_release_issue](https://usethis.r-lib.org/reference/use_release_issue.html) to see if there is anything we are missing in this issue for CRAN submission preparation~~ - [x] ~~Create a [website](https://r-pkgs.org/website.html#introduction) for the package using the R package [pkgdown](https://pkgdown.r-lib.org/articles/pkgdown.html) since it says "you'll get a decent website without any additional work" and you can "get a basic website up and running in just a couple of minutes"~~

Steps to follow after CRAN acceptance

Steps

- Accepted 🎉 - `git push` - [usethis::use_github_release()](https://usethis.r-lib.org/reference/use_github_release.html) - [usethis::use_dev_version()](https://usethis.r-lib.org/reference/use_version.html) - `git push` - promote package

nateschor commented 1 year ago

@jmshapir yes I can take the lead on releasing to CRAN!

jmshapir commented 1 year ago

Great, thanks @nateschor!

We should probably prioritize the install errors in #25, but once those are resolved it would be great if you can turn to #28. Thank you!

jorpppp commented 1 year ago

Just tagging @Constantino-Carreto-Romero: when the release to CRAN is done, please try to install eventstudyr in your Banxico computer, both from CRAN and from GitHub.

If IT gives you a hard time because you're trying to install from GitHub, tell them we wrote the package.

nateschor commented 1 year ago

@jmshapir FYI that in the opening comment I've been going through the R Packages book and writing down everything we need to or might want to do before submitting to CRAN.

After I've finished the brain dump I'll think more about how to reasonably organize the tasks and then we can iterate on what things to:

Thanks!

Edit: I finished my brain dump and first pass at organizing. Me and the RAs will discuss at our call tomorrow, and we can discuss at our group call on Monday if needed

nateschor commented 1 year ago

@mzwu @ew487 @rcalvo12 I couldn't help myself...

image

...but I do think adding a hex with the package would be nice! It was pretty easy to make, code is below

code

```r pacman::p_load( eventstudyr, hexSticker, ggplot2 ) estimates_ols <- EventStudy( estimator = "OLS", data = df_sample_dynamic, outcomevar = "y_smooth_m", policyvar = "z", idvar = "id", timevar = "t", controls = "x_r", FE = TRUE, TFE = TRUE, post = 3, overidpost = 5, pre = 2, overidpre = 4, normalize = - 3 ) plt_ols <- EventStudyPlot( estimates = estimates_ols, pre_event_coeffs = FALSE, post_event_coeffs = FALSE ) + theme_transparent() plt_ols$layers[[4]] <- geom_point(color = "#006600", size = .5) hex_sticker <- sticker(plt_ols, package = "eventstudyr", filename = "eventstudry_hex.png", p_size=8, s_x=.92, s_y=.9, s_width=1.5, s_height=1.5, p_y = 1.7, h_fill = "#af8dc3") plot(hex_sticker) ```

jorpppp commented 1 year ago

@nateschor I like it! A couple of suggestions:

jmshapir commented 1 year ago

From https://github.com/JMSLab/eventstudyr/issues/20#issuecomment-1483078251:

@jmshapir @jorpppp @chansen776 @santiagohermo just checking if anyone needs any type of disclaimer (such as funding) present in the repo for their institution/bank? @SimonFreyaldenhoven is looking into it for me and him.

Thanks!

@nateschor if it's standard to have acknowledgments in a repo like this then I would add mine from the chapter. If not, it seems fine to omit since we are anyway citing the chapter. Thanks!

SimonFreyaldenhoven commented 1 year ago

@nateschor I think we're also good without any particular disclaimer.

nateschor commented 1 year ago

@nateschor if it's standard to have acknowledgments in a repo like this then I would add mine from the chapter. If not, it seems fine to omit since we are anyway citing the chapter. Thanks!

@jmshapir I haven't come across many R package GitHub repos that have an acknowledgements section. The package r5r does, though.

In light of this, do you want to include all (or part of) the chapter? Or you're okay with the README citation as it currently is?

Thanks!

jmshapir commented 1 year ago

@nateschor in that case I'm fine with the citation as is, thanks.

jmshapir commented 1 year ago

Per call: @nateschor will take a first pass here and let us know where we can pitch in.

santiagohermo commented 1 year ago

Some thoughts that came up during the call @nateschor.

Alternatives for hosting the documentation:

The "seed problem". A proposed solution is here.

jmshapir commented 1 year ago

The "seed problem". A proposed solution is here.

@santiagohermo thanks! I actually think just pulling the seed out of the function might be the cleanest solution, per @SimonFreyaldenhoven @jorpppp's suggestions on the call. Then we can just set a seed in the examples and make explicit in the documentation that the AddSupT function is simulation-based and so is seed-dependent.

But I'm fine with something like what you've proposed, too.

@nateschor fyi.

nateschor commented 1 year ago

Per call: @nateschor will take a first pass here and let us know where we can pitch in.

@jmshapir I can work on this all day tomorrow and then all day one day next week.

For tomorrow, I'll focus on the essential and high impact tasks where I think I can be most helpful, and then I'll provide an update at the end of the day tomorrow.

How does that sound?

@santiagohermo fyi

jmshapir commented 1 year ago

@nateschor sounds perfect thanks!

nateschor commented 1 year ago

@jmshapir @santiagohermo based on https://github.com/JMSLab/eventstudyr/issues/28#issuecomment-1485817169 I made the following changes in https://github.com/JMSLab/eventstudyr/commit/145dd4a1354f1d1a63a729af2535f8e8b77a5c95:

Please let me know whether or not this implementation works for you, thanks!

santiagohermo commented 1 year ago

Thanks @nateschor! Your changes to tackle the seed problem in https://github.com/JMSLab/eventstudyr/issues/28#issuecomment-1487498167 look great to me.

nateschor commented 1 year ago

I'll focus on the essential and high impact tasks where I think I can be most helpful, and then I'll provide an update at the end of the day

@santiagohermo I addressed most of the "Must" and "Should" tasks from the opening comment. A few notes:

I don't have any more bandwidth this week to work on the package, but I'll have a day to work on it next week! I can still respond to messages here, though, and we can discuss in more detail during our call Friday.

@jmshapir @rcalvo12 @ew487 fyi

Thank you!

santiagohermo commented 1 year ago

Thanks @nateschor! Some replies below:

@santiagohermo I addressed most of the "Must" and "Should" tasks from the opening comment. A few notes:

  • I currently have version 7.1.2 of roxygen2 and was having some trouble installing 7.2.3, the required version for our package. I get the warning "check() will not re-document this package" when running devtools::check(). If you have ≥7.2.3, would you be able to confirm that you don't get any notes, warnings, or errors when running devtools::check()?

I have version 7.2.3 of roxgen2. When I run devtools::check() I get the following:

image

No notes, warnings or errors!

  • I'm still not completely certain on the proper way to ship a vignette with the package. Based on this, I think we have it correct with no rendered vignette shipped, our .Rmd file in the vignettes folder, and the additions to the DESCRIPTION from here. However, I still see a message when I run devtools::check() that says "Removed empty directory 'eventstudyr/vignettes' " which doesn't sound like behavior we want. Would you be able to help with looking into how we should ship the vignette?

  • Please let me know if you think it's worthwhile to address the 2 remaining tasks in "Should" before we submit to CRAN

  • Please let me know if you think any of the "Would Like" tasks should be escalated to "Should" or "Must"

These ones seem quick to implement, maybe we can just do them.

On this one:

Following my earlier comment I think that I can tackle a "large" issue that revises PrepareLeads/Lags, including this point. Happy to open an issue for this after CRAN.

nateschor commented 1 year ago

Thanks @santiagohermo! Replies:

On renv. How hard is it to set it up? I think that, if it's not strictly necessary for CRAN, we can skip it for now. If as we go we learn that package updates tend to break the code often I'd be in favor of setting it up.

I've set it up a few times for non-package .Rproj projects, but never with a package. It doesn't look terribly hard, see here, but since it's not strictly necessary for CRAN I'm good to skip it for now!

On PrepareLeads and PrepareLags. I think that there are many good options for leading/lagging variables in R (I often use data.table::shift()), so I don't see a strong case for making them user-facing. The last point in "After Going Live" asks for a revision of these functions. I'd be happy to tackle that point and think again about making them user-facing or not.

I'm in favor of making PrepareLeads and PrepareLags not user-facing for our initial CRAN submission and then revisiting if we want to make them user-facing. Having you tackle the points in "After Going Live" would be great, thank you!

On TestLinear. I think that a user may plausibly want to test the pre-trend and leveling-off hypotheses outside of the plotting, maybe to add them to a table. I would favor making it user-facing.

Keeping TestLinear as user-facing in our initial release sounds good to me!

These ones seem quick to implement, maybe we can just do them.

  • Set up package citation
  • Add link to the eventstudyr repo in DESCRIPTION, we can also add a link that is helpful for bug reportting

Sounds good! Do you have bandwidth to implement? If not, I can implement them next week.

  • Potentially merge the documentation for PrepareLeads and PrepareLags since they're so similar

Following my earlier comment I think that I can tackle a "large" issue that revises PrepareLeads/Lags, including this point. Happy to open an issue for this after CRAN.

This sounds good to me!

I'm still not completely certain on the proper way to ship a vignette with the package. Based on this, I think we have it correct with no rendered vignette shipped, our .Rmd file in the vignettes folder, and the additions to the DESCRIPTION from here. However, I still see a message when I run devtools::check() that says "Removed empty directory 'eventstudyr/vignettes' " which doesn't sound like behavior we want. Would you be able to help with looking into how we should ship the vignette?

Do you have bandwidth to help with figuring out how to ship the vignette?

Thank you!

nateschor commented 1 year ago

Per call: I'll reorganize the tasks in the opening comment.

We should be ready to open a PR next week!

Edit: I put a checkmark and strikethrough for tasks that we'll either:

santiagohermo commented 1 year ago

Thanks @nateschor!

Some completed tasks:

Set up package citation

Done in https://github.com/JMSLab/eventstudyr/commit/da4e9f7d46417c8c001d239980abba347ac43197. The citation follows the README and looks like this:

image

Add link to the eventstudyr repo in DESCRIPTION, we can also add a link that is helpful for bug reportting

Done in https://github.com/JMSLab/eventstudyr/commit/2de5dce27fe21339c173ddbc7290b28bf4787470.

Other things we discussed above:

nateschor commented 1 year ago

@santiagohermo this looks great, thank you! This all looks great. Later today I'll check that everything is working as expected and then open a PR with you and @jmshapir as reviewers

Edit:

About the vignette. I think I figured out the problem! When I created an empty vignette using usethis::use_vignette("my-vignette") things seem to work, even when it had the same content as in vignettes/eventstudyr.Rmd. So I decided to try changing the name of the file. I implemented in https://github.com/JMSLab/eventstudyr/commit/328d7c8cfa691e806d3f36a68f709d0eb13514a8 and now the vignette is shipped in my computer. Can you check?

This worked!

nateschor commented 1 year ago

Thread continues in #32

nateschor commented 1 year ago

Summary:

In this issue we prepared eventstudyr for initial submission to CRAN. The changes we made are checked off in the opening comment, while changes we might want to address in future issues are checked off and have a strikethrough. We updated our license to cite AddSuptBand but will modify our approach if the sup-t code becomes available on CRAN, see https://github.com/ryanedmundkessler/suptCriticalValue/issues/2

The only changes we made that could impact a user are:

Next steps are outlined in https://github.com/JMSLab/eventstudyr/pull/32#pullrequestreview-1377856245

Merged into main in https://github.com/JMSLab/eventstudyr/commit/04691ee6beb5815114f0dabb285794e7291a3bf7

Stable link to issue branch here

Stable link to our package development workflow here