search

asheshrambachan / HonestDiD

Robust inference in difference-in-differences and event study designs
Other
175 stars 45 forks source link

Clean up repo #30

Closed mcaceresb closed 1 year ago

mcaceresb commented 1 year ago

The branch cleans up the repo so it can be submitted to CRAN (passes R CMD check --as-cran). Changes are:

@asheshrambachan @jonathandroth Questions

jonathandroth commented 1 year ago

@Ashesh Rambachan @.***> any idea on how %do% is being used here?

On Sat, May 20, 2023 at 11:36 AM Mauricio Caceres Bravo < @.***> wrote:

The branch cleans up the repo so it can be submitted to CRAN (passes R CMD check --as-cran). Changes are:

@asheshrambachan https://github.com/asheshrambachan @jonathandroth https://github.com/jonathandroth Questions

-

When I run the CRAN checks I get this note: Namespace in Imports field not imported from: ‘doParallel’. It might be ignorable, but I'm trying to figure out where doParallel is used. Does it import %do%? Is it a back-end for foreach? I don't have experience with doParallel, sorry if it's meant to be clear!

Several files in man have placeholders for keywords. How to fill them?

You can view, comment on, or merge this pull request online at:

https://github.com/asheshrambachan/HonestDiD/pull/30 Commit Summary

File Changes

(39 files https://github.com/asheshrambachan/HonestDiD/pull/30/files)

Patch Links:

— Reply to this email directly, view it on GitHub https://github.com/asheshrambachan/HonestDiD/pull/30, or unsubscribe https://github.com/notifications/unsubscribe-auth/AE6EXFENR4NDIVOUI2AM2HLXHDQI3ANCNFSM6AAAAAAYIZUXYM . You are receiving this because you were mentioned.Message ID: @.***>

asheshrambachan commented 1 year ago

%do% is loaded in the foreach which I think gets used to loop over alternative values of M or Mbar in the sensitivity analysis results functions. doParallel was how we originally implemented parallelized versions via %dopar%.

mcaceresb commented 1 year ago

@asheshrambachan @jonathandroth Since both are imported from foreach I'll just make doParallel a suggested package instead of required.

I'll also delete the placeholder keywords for now. So if there's nothing else I'll implement those changes and merge the PR tomorrow (traveling rn). After that it should be set for CRAN (unless you want to add tests).

mcaceresb commented 1 year ago

@asheshrambachan @jonathandroth Done! I also updated the manual (doc/manual.pdf) but devtools/R CMD tell me that it's not standard to include a doc directory. I tried looking around and I'm not sure where else to put the manual, however.

jonathandroth commented 1 year ago

Just wanted to follow up on the status of preparing this for CRAN. Is the package ready to go to CRAN? Or is there something we're waiting on? If it's ready, we should submit it!

mcaceresb commented 1 year ago

It should be ready.

mcaceresb commented 1 year ago

@jonathandroth Just checking in in case you were waiting for me? I believe Ashesh has to submit the package, tho. See here.

devtools::build() should make the requisite .tar.gz file that he'll have to upload.

asheshrambachan commented 1 year ago

Thanks @mcaceresb! I didn't realize I had to submit the package. I'll do it this weekend and let you know if I run into any issues.

mcaceresb commented 1 year ago

@asheshrambachan I haven't submitted a CRAN package before. It looks to me that the package maintainer has to submit but if that wasn't the case LMK for future reference.

jonathandroth commented 1 year ago

Did you submit this Ashesh?

On Fri, Jul 7, 2023 at 7:29 PM Mauricio Caceres Bravo < @.***> wrote:

@asheshrambachan https://github.com/asheshrambachan I haven't submitted a CRAN package before. It looks to me that the package maintainer has to submit but if that wasn't the case LMK for future reference.

— Reply to this email directly, view it on GitHub https://github.com/asheshrambachan/HonestDiD/pull/30#issuecomment-1626358046, or unsubscribe https://github.com/notifications/unsubscribe-auth/AE6EXFGWDD3EW7SUHIJZQTDXPCLT3ANCNFSM6AAAAAAYIZUXYM . You are receiving this because you were mentioned.Message ID: @.***>

jonathandroth commented 1 year ago

?

On Tue, Jul 11, 2023 at 10:04 AM Jonathan Roth @.***> wrote:

Did you submit this Ashesh?

On Fri, Jul 7, 2023 at 7:29 PM Mauricio Caceres Bravo < @.***> wrote:

@asheshrambachan https://github.com/asheshrambachan I haven't submitted a CRAN package before. It looks to me that the package maintainer has to submit but if that wasn't the case LMK for future reference.

— Reply to this email directly, view it on GitHub https://github.com/asheshrambachan/HonestDiD/pull/30#issuecomment-1626358046, or unsubscribe https://github.com/notifications/unsubscribe-auth/AE6EXFGWDD3EW7SUHIJZQTDXPCLT3ANCNFSM6AAAAAAYIZUXYM . You are receiving this because you were mentioned.Message ID: @.***>

asheshrambachan commented 1 year ago

Thanks for the reminder!

I started the process of submitting the package. As part of the submission, we have to run some checks to see if there any "bad practices" in the package's implementation -- this just involves running devtools::build() or R CMD check --as-cran after building the `.tar.gz' file. My understanding of the CRAN policy is that we have to address all notes raised by this check.

@mcaceresb when I ran this check, I got the following three notes in the screenshot below: Screenshot 2023-07-17 at 6 09 29 PM Do you know how to fix these notes?

The CRAN policy also says "If there are warnings or notes you cannot eliminate (for example because you believe them to be spurious) send an explanatory note as part of your covering email, or as a comment on the submission form." So if we can't fix them, then we can just include an explanatory note in the comment on the submission.

mcaceresb commented 1 year ago

@asheshrambachan Right so let's take these issues in turn:

  1. doParallel: I thought this was fixed, so I'm not sure what it's about. I also don't get this error locally, and it seems it might be an OS-specific thing. I can just get rid of doParallel as a suggested package? LMK
  2. doc: This contains the manual. I'm not sure if you've linked to it ever, but apparently doc is not supposed to be a directory included in the package submission. If you're good I can just delete it. LMK.
  3. no visible binding: dplyr refers to variables in context, so all those variables don't exist, so R, complains, but the code runs because dplyr looks for them inside the data. I don't know how to fix those notes, but the package should run (because dplyr is working correctly).
asheshrambachan commented 1 year ago

Thanks @mcaceresb! On the issues:

  1. Yes, I think it's easiest to just get rid of doParallel as the suggested package at this point.
  2. Yes, let's go ahead and delete the manual. We never really directed users towards it, and I think we can safely remove given the detailed vignette we now have.
  3. Got it -- that makes sense. Per my post here, we can provide an explanatory note with the submission so I can mention that we are aware of this note and it does not affect the running of the package.

Let me know when you made those two changes and I will submit!

jonathandroth commented 1 year ago

Fantastic, thank you both!

On Fri, Jul 21, 2023 at 3:56 PM Ashesh Rambachan @.***> wrote:

Thanks @mcaceresb https://github.com/mcaceresb! On the issues:

  1. Yes, I think it's easiest to just get rid of doParallel as the suggested package at this point.
  2. Yes, let's go ahead and delete the manual. We never really directed users towards it, and I think we can safely remove given the detailed vignette we now have.
  3. Got it -- that makes sense. Per my post here https://github.com/asheshrambachan/HonestDiD/pull/30#issuecomment-1638954706, we can provide an explanatory note with the submission so I can mention that we are aware of this note and it does not affect the running of the package.

Let me know when you made those two changes and I will submit!

— Reply to this email directly, view it on GitHub https://github.com/asheshrambachan/HonestDiD/pull/30#issuecomment-1646175561, or unsubscribe https://github.com/notifications/unsubscribe-auth/AE6EXFCWIYBWFF7HTHRA6G3XRLNFHANCNFSM6AAAAAAYIZUXYM . You are receiving this because you were mentioned.Message ID: @.***>

mcaceresb commented 1 year ago

@asheshrambachan Done in #35; LMK if it fixes the issues.

asheshrambachan commented 1 year ago

Thanks @mcaceresb! I think the issues are fixed. The only one that remains is the no visible binding:dplyr but I'll include a note with the submission.

It looks like the CRAN submissions are offline until Aug 7th due to maintenance work (see screenshot). I'll come back and submit the package when it's back online.

Screenshot 2023-07-22 at 11 53 36 AM

jonathandroth commented 1 year ago

Good thing you don't have anything going on between now and Aug 7, Ashesh :)

On Sat, Jul 22, 2023 at 12:03 PM Ashesh Rambachan @.***> wrote:

Thanks @mcaceresb https://github.com/mcaceresb! I think the issues are fixed. The only one that remains is the no visible binding:dplyr but I'll include a note with the submission.

It looks like the CRAN submissions are offline until Aug 7th due to maintenance work (see screenshot). I'll come back and submit the package when it's back online.

[image: Screenshot 2023-07-22 at 11 53 36 AM] https://user-images.githubusercontent.com/29416461/255338887-f257d839-c42e-4fac-920d-986102ebaefb.png

— Reply to this email directly, view it on GitHub https://github.com/asheshrambachan/HonestDiD/pull/30#issuecomment-1646617159, or unsubscribe https://github.com/notifications/unsubscribe-auth/AE6EXFAQZ2MYZHGG3TQCU5TXRP2V3ANCNFSM6AAAAAAYIZUXYM . You are receiving this because you were mentioned.Message ID: @.***>

asheshrambachan commented 1 year ago

Submitted this morning! Let's see what happens. Screenshot 2023-08-14 at 8 23 56 AM

jonathandroth commented 1 year ago

Word, thanks!

On Mon, Aug 14, 2023, 8:24 AM Ashesh Rambachan @.***> wrote:

Submitted this morning! Let's see what happens. [image: Screenshot 2023-08-14 at 8 23 56 AM] https://user-images.githubusercontent.com/29416461/260446271-742bf25f-2fe0-4b8b-b652-79dd7cc29e3a.png

— Reply to this email directly, view it on GitHub https://github.com/asheshrambachan/HonestDiD/pull/30#issuecomment-1677222068, or unsubscribe https://github.com/notifications/unsubscribe-auth/AE6EXFBYFUWYYG7WQEQT24TXVIKH5ANCNFSM6AAAAAAYIZUXYM . You are receiving this because you were mentioned.Message ID: @.***>

asheshrambachan commented 1 year ago

@mcaceresb the CRAN maintainers say we need to address the no visible binding:dplyr note from the build check. They wrote me this email below: Screenshot 2023-08-14 at 9 16 53 AM

mcaceresb commented 1 year ago

Addressed in #40.

asheshrambachan commented 1 year ago

Thanks @mcaceresb!

Resubmitted to CRAN.

Screenshot 2023-08-24 at 7 52 34 PM
jonathandroth commented 1 year ago

Thanks both!

On Thu, Aug 24, 2023, 7:53 PM Ashesh Rambachan @.***> wrote:

Thanks @mcaceresb https://github.com/mcaceresb!

Resubmitted to CRAN. [image: Screenshot 2023-08-24 at 7 52 34 PM] https://user-images.githubusercontent.com/29416461/263130942-e90998d4-f382-4bad-8257-7410952fec59.png

— Reply to this email directly, view it on GitHub https://github.com/asheshrambachan/HonestDiD/pull/30#issuecomment-1692560006, or unsubscribe https://github.com/notifications/unsubscribe-auth/AE6EXFCVQHYRY7VJCTNDPBLXW7SQDANCNFSM6AAAAAAYIZUXYM . You are receiving this because you were mentioned.Message ID: @.***>

asheshrambachan commented 1 year ago

Got the following feedback from CRAN. Nothing seems like it would be major to fix.

Screenshot 2023-08-31 at 4 07 26 PM
jonathandroth commented 1 year ago

Boy, they are sticklers!

Mauricio, might you have some RA time this semester to do the final clean-up on this?

On Thu, Aug 31, 2023 at 4:08 PM Ashesh Rambachan @.***> wrote:

Got the following feedback from CRAN. Nothing seems like it would be major to fix. [image: Screenshot 2023-08-31 at 4 07 26 PM] https://user-images.githubusercontent.com/29416461/264768812-4026329d-7f51-45a1-b878-8db4bfc8d5bf.png

— Reply to this email directly, view it on GitHub https://github.com/asheshrambachan/HonestDiD/pull/30#issuecomment-1701712596, or unsubscribe https://github.com/notifications/unsubscribe-auth/AE6EXFB45NBF3M6XOIBQPGDXYDVLTANCNFSM6AAAAAAYIZUXYM . You are receiving this because you were mentioned.Message ID: @.***>

mcaceresb commented 1 year ago

@jonathandroth @asheshrambachan I should be able to get to this next week when the semester starts.

I have no idea what the first one means, but hopefully it ends up being documented somewhere. The rest I understand (though they seem to be using some type of linter; they ought to just make that part of the submission process I think instead of manual back and forth).

Anyway, I take it you don't have very strong feelings about what examples I give in the .Rd files?

jonathandroth commented 1 year ago

Sounds great, thanks Mauricio!

On Thu, Aug 31, 2023, 6:05 PM Mauricio Caceres Bravo < @.***> wrote:

@jonathandroth https://github.com/jonathandroth @asheshrambachan https://github.com/asheshrambachan I should be able to get to this next week when the semester starts.

I have no idea what the first one means, but hopefully it ends up being documented somewhere. The rest I understand (though they seem to be using some type of linter; they ought to just make that part of the submission process I think instead of manual back and forth).

Anyway, I take it you don't have very strong feelings about what examples I give in the .Rd files?

— Reply to this email directly, view it on GitHub https://github.com/asheshrambachan/HonestDiD/pull/30#issuecomment-1701843522, or unsubscribe https://github.com/notifications/unsubscribe-auth/AE6EXFFDGGKC3XRS3H4VBM3XYEDCTANCNFSM6AAAAAAYIZUXYM . You are receiving this because you were mentioned.Message ID: @.***>

mcaceresb commented 1 year ago

@jonathandroth @asheshrambachan I started to answer everything here in #42. Just need to add examples to each .Rd file.

In the meantime:

jonathandroth commented 1 year ago

Both look fine to me!

On Mon, Sep 11, 2023 at 9:50 PM Mauricio Caceres Bravo < @.***> wrote:

@jonathandroth https://github.com/jonathandroth @asheshrambachan https://github.com/asheshrambachan I started to answer everything here https://github.com/asheshrambachan/HonestDiD/pull/30#issuecomment-1701712596 in #42 https://github.com/asheshrambachan/HonestDiD/pull/42. Just need to add examples to each .Rd file.

In the meantime:

— Reply to this email directly, view it on GitHub https://github.com/asheshrambachan/HonestDiD/pull/30#issuecomment-1714845720, or unsubscribe https://github.com/notifications/unsubscribe-auth/AE6EXFBDG4U5VZWNN2XDYULXZ65XNANCNFSM6AAAAAAYIZUXYM . You are receiving this because you were mentioned.Message ID: @.***>