PR for #31: Revise functions `PrepareLeads`, `PrepareLags`, and `GetFirstDifferences`

[santiagohermo]

Closes #31. We implemented the following changes:

• Replace GetFirstDifferences with ComputeFirstDifferences which computes differences robustly to holes in the time variable
• Replace PrepareLeads and PrepareLags with ComputeShifts which is also robust to holes in time variable
• Improvements to PrepareModelFormula that impacted the display of coefficients in the output of EventStudy
• Updated relevant tests

Who wants to take this review @nateschor @jmshapir @SimonFreyaldenhoven? We need to check the code, and make sure the documentation was updated correctly.

One more to-do from #31 is to decide whether the new functions should be user-facing or not.

Finally, if we are going to make a new release we need to update the version number in DESCRIPTION.

[SimonFreyaldenhoven]

@santiagohermo Thanks! just FYI - I'm pretty swamped this week, and I think Nathan also likely won't be able to take a look until next week.

[santiagohermo]

Thanks for the heads-up @SimonFreyaldenhoven! No worries, no rush on my side to close this one.

[jmshapir]

@santiagohermo thanks!

Who wants to take this review @nateschor @jmshapir @SimonFreyaldenhoven? We need to check the code, and make sure the documentation was updated correctly.

I'm happy to take a first pass at a review.

One more to-do from #31 is to decide whether the new functions should be user-facing or not.

I don't have strong feelings on this but on the principle of keeping documentation simple I would suggest we err on the side of not making functions user-facing unless we have reason to expect a significant number of users will want to use them.

Finally, if we are going to make a new release we need to update the version number in DESCRIPTION.

Does CRAN impose any rules on version numbering? For example if we previously submitted v1.0.2 to CRAN can our next submission be called, e.g., v.1.0.5?

I think the answer to that might affect the release policy we want to adopt.

[santiagohermo]

Note @jmshapir @nateschor. You might have received a notification that Actions failed. There were a few small errors that I fixed in https://github.com/JMSLab/eventstudyr/pull/38/commits/b9c568b72ed40fd72c82b956d6c815e69597734f, so now this should be fixed. (New Actions test running as I write this.)

[santiagohermo]

Note @jmshapir @nateschor. You might have received a notification that Actions failed. There were a few small errors that I fixed in b9c568b, so now this should be fixed. (New Actions test running as I write this.)

Looks like the ubuntu check failed, apparently when installing packages.

I will try to re-do. If it fails again we will have to spend some time trying to fix it.

[nateschor]

@santiagohermo I'll have some time tomorrow and Wednesday to review this PR!

[santiagohermo]

Thanks @nateschor!

Looks like the problematic ubuntu check now passed, so we are good on that front!

[jmshapir]

Per call:

• @nateschor will review
• @mzwu will test new functionality
• @Constantino-Carreto-Romero @jorpppp will compare to Stata

We'll focus on avoiding bad "side effects" and leave conveniences to the roadmap.

[mzwu]

Some comments from testing:

• ComputeFirstDifferences does not check that return_df must be a logical
• Checking for character types is not consistent: ComputeFirstDifferences checks each one while ComputeShifts uses a loop
• Inconsistent periods at the end of parameter descriptions: ComputeFirstDifferences is missing a period here
• PrepareModelFormula is missing checks for the proxy parameter: should be a character and specified if and only if the estimator is FHS

Will continue to test over the next few days, but here are some initial things I noticed! @jmshapir @santiagohermo @nateschor

[nateschor]

@jmshapir @santiagohermo regarding CRAN rules on versioning from https://github.com/JMSLab/eventstudyr/pull/38#pullrequestreview-1426848114:

I think it is fine if we increment our patch number by more than 1 between CRAN submissions. The example you gave of going from 1.0.2 to 1.0.5 should be good. See the table for the usethis package here which, for example, has its CRAN versions go from 1.6.1 to 1.6.3 and 2.1.0 to 2.1.2

[santiagohermo]

Thanks for those great comments @mzwu! I think I addressed them in https://github.com/JMSLab/eventstudyr/pull/38/commits/4dbbde0fc65e1c501df47afdd15c4bf0f0b31f1c, let me know if I misinterpreted anything or you have other concerns here.

[santiagohermo]

Thanks @nateschor! I will take a look at your comments and let you know once I'm done.

[jmshapir]

@jmshapir @santiagohermo regarding CRAN rules on versioning from #38 (review):

I think it is fine if we increment our patch number by more than 1 between CRAN submissions. The example you gave of going from 1.0.2 to 1.0.5 should be good. See the table for the usethis package here which, for example, has its CRAN versions go from 1.6.1 to 1.6.3 and 2.1.0 to 2.1.2

@nateschor thanks! In that case I think that on completion of this pull we should make a release and increment the version number.

[santiagohermo]

I'm done with my replies @nateschor, thanks for the helpful review!

[nateschor]

@santiagohermo thanks for the helpful replies! I have just one remaining comment in https://github.com/JMSLab/eventstudyr/pull/38#discussion_r1196601066, and then I'll approve the PR 👍

[santiagohermo]

Thanks @nateschor! I replied to https://github.com/JMSLab/eventstudyr/pull/38#discussion_r1196601066. Also note that this comment is still open.

[jmshapir]

@mzwu if all your comments are addressed and you don't plan to post more, can you please approve the pull? Thanks!

[mzwu]

@jmshapir Just approved, thanks!

[jmshapir]

@mzwu thanks!

@santiagohermo if we haven't heard otherwise from @Constantino-Carreto-Romero @jorpppp by first thing Monday, I think we can go ahead and merge. We can always open a follow-up to fix any specific concerns that may come up later.

[santiagohermo]

Thanks @jmshapir! I'll proceed to wrap up this issue and merge. I will also draft a new release for @nateschor to review.

[Constantino-Carreto-Romero]

I'm very sorry we could not review, we've been pretty swamped these days.

[santiagohermo]

Summary here

[santiagohermo]

Quick question @jmshapir @nateschor. For the new release do we want to go to 1.0.3 or 1.1.0? Also, I forgot to change the version number in DESCRIPTION so I will need to make a commit directly to main, apologies!

The draft of the release is here.

[jmshapir]

Quick question @jmshapir @nateschor. For the new release do we want to go to 1.0.3 or 1.1.0?

I defer to @nateschor!

Also, I forgot to change the version number in DESCRIPTION so I will need to make a commit directly to main, apologies!

I think it's fine to open a second issue branch to tackle this small change.

[santiagohermo]

Thanks @jmshapir! I opened #39 for this.