Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Clean up repo #30

Merged
merged 13 commits into from
May 30, 2023
Merged

Clean up repo #30

merged 13 commits into from
May 30, 2023

Conversation

mcaceresb
Copy link
Collaborator

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

@asheshrambachan @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?

mcaceresb added 12 commits May 10, 2023 22:41
@mcaceresb
Added Rglpk as a dependency
5f53c4a
@mcaceresb
Removed tidyverse as package loading
9e1da67
@mcaceresb
Added mvtnorm as dependency
1339702
@mcaceresb
Squashed commit of the following:
14f38ac 
commit 3c3c4c53588b390f7ca8d5c78d5dff5cae7dc5ec
Author: Mauricio Caceres <mauricio.caceres.bravo@gmail.com>
Date:   Wed Apr 5 14:53:19 2023 -0400

    More explicit calls

commit 9d01fe1
Author: Mauricio Caceres <mauricio.caceres.bravo@gmail.com>
Date:   Sat Apr 1 20:58:22 2023 -0400

    Re-compiled from R studio; closes asheshrambachan#24

commit b8cf9f2
Author: Mauricio Caceres <mauricio.caceres.bravo@gmail.com>
Date:   Sat Apr 1 20:42:14 2023 -0400

    Added explicit calls to built-ins
@mcaceresb
Squashed commit of the following:
c310d29 
commit b9d8af8
Author: Mauricio Caceres <mauricio.caceres.bravo@gmail.com>
Date:   Thu May 11 01:07:47 2023 -0400

    Moved intro of staggered example into README

commit 02b541b
Author: Mauricio Caceres <mauricio.caceres.bravo@gmail.com>
Date:   Thu May 11 01:01:11 2023 -0400

    Fixed potential integer overflow

commit b4767b7
Author: Mauricio Caceres <mauricio.caceres.bravo@gmail.com>
Date:   Thu May 11 00:56:23 2023 -0400

    Cleaned and moved honest_did function to own file
@mcaceresb
Several fixes/edits to pass CRAN check
918c527 
- Added matrixStats to Imports
- Deleted ROI
- Renamed doc -> vignettes
- .compute_IDset_DeltaRMB_fixedS: biasDrection -> biasDirection
- .create_A_M: A_I -> A_M
- DeltaSD_upperBound_Mpre: stata -> stats
- Changed title to Title Case
- https://jonathandroth.github.io -> https://www.jonathandroth.com/
- Aligned man files with function definitions:
    - findOptimalFLCI
    - createSensitivityResults_relativeMagnitudes
    - computeConditionalCS_DeltaSDRMB
    - computeConditionalCS_DeltaSDM
    - computeConditionalCS_DeltaSDB
    - computeConditionalCS_DeltaSD
- Added deltaSD.png, README to .Rbuildignore
- Added VignetteBuilder and knitr to DESCRIPTION
@mcaceresb
Fixed Ashesh e-mail; removed library statements
a07518d
@mcaceresb
Merge branch 'asheshrambachan:master' into master
e444da7
@mcaceresb
Merge branch 'asheshrambachan:master' into master
801818b
@mcaceresb
Merge branch 'master' into cran-commit
ac84995
@mcaceresb
Bumped version #
9ab47c9
@mcaceresb
Added rmarkdown to build vignettes
2a0ea8f
@jonathandroth
Copy link
Collaborator

jonathandroth commented May 29, 2023 via email

@asheshrambachan
Copy link
Owner

%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
Copy link
Collaborator Author

@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
Copy link
Collaborator Author

@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.

@mcaceresb mcaceresb merged commit a1e7126 into asheshrambachan:master May 30, 2023
@mcaceresb mcaceresb deleted the cran-commit branch May 30, 2023 18:39
@jonathandroth
Copy link
Collaborator

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
Copy link
Collaborator Author

It should be ready.

@mcaceresb
Copy link
Collaborator Author

@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
Copy link
Owner

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
Copy link
Collaborator Author

@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
Copy link
Collaborator

jonathandroth commented Jul 11, 2023 via email

@jonathandroth
Copy link
Collaborator

jonathandroth commented Jul 17, 2023 via email

@asheshrambachan
Copy link
Owner

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
Copy link
Collaborator Author

mcaceresb commented Jul 21, 2023 •
edited
Loading

@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
Copy link
Owner

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
Copy link
Collaborator

jonathandroth commented Jul 21, 2023 via email

@mcaceresb
Copy link
Collaborator Author

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

@asheshrambachan
Copy link
Owner

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
Copy link
Collaborator

jonathandroth commented Jul 24, 2023 via email

@asheshrambachan asheshrambachan mentioned this pull request Aug 14, 2023
Merged
@asheshrambachan
Copy link
Owner

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

@jonathandroth
Copy link
Collaborator

jonathandroth commented Aug 14, 2023 via email

@asheshrambachan
Copy link
Owner

@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 added a commit to mcaceresb/HonestDiD that referenced this pull request Aug 21, 2023
@mcaceresb
asheshrambachan#30 Moved to using .data in dplyr; m <- NULL for foreach
c91da5d
@mcaceresb mcaceresb mentioned this pull request Aug 21, 2023
Merged
@mcaceresb
Copy link
Collaborator Author

Addressed in #40.

asheshrambachan added a commit that referenced this pull request Aug 24, 2023
@asheshrambachan
Merge pull request #40 from mcaceresb/cran-commit-round3
bb2c7b6 
Pull Request follow-up for #30: Take care of undefined globals warning
@asheshrambachan
Copy link
Owner

Thanks @mcaceresb!

Resubmitted to CRAN.
Screenshot 2023-08-24 at 7 52 34 PM

@jonathandroth
Copy link
Collaborator

jonathandroth commented Aug 24, 2023 via email

@asheshrambachan
Copy link
Owner

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
Copy link
Collaborator

jonathandroth commented Aug 31, 2023 via email

@mcaceresb
Copy link
Collaborator Author

@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
Copy link
Collaborator

jonathandroth commented Aug 31, 2023 via email

@mcaceresb mcaceresb mentioned this pull request Sep 12, 2023
Merged
6 tasks
@mcaceresb
Copy link
Collaborator Author

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

In the meantime:

  • Can you check the description here is OK?
  • I gave both of you author and copyright and Ashesh maintainer in the Authors@R field. If there's a different configuration I should use LMK.

@jonathandroth
Copy link
Collaborator

jonathandroth commented Sep 12, 2023 via email

mcaceresb added a commit that referenced this pull request Oct 22, 2023
@mcaceresb
Pull Request follow-up for #30: Various minor fixes (#42)
10c13d6 
* Added 'value' to man/constructOriginalCS.Rd

* Closed [ in description

* Changed all T/F to TRUE/FALSE

* Removed all standardtext from .Rd files

* Give Ashesh and Jon author and copyright; Ashesh creator/maintainer

* Added simple examples for main user funs

* Minor fix

* Reference vignette name directly in simple examples

* Direct users to github README in help files
mcaceresb added a commit that referenced this pull request Mar 10, 2024
@mcaceresb
Pull Request follow-up for #30: Various formatting issues (#54)
bcb3ec4 
* Added basic tests

* Fixed minor typo; fixed tests

* Added pandoc to gh actions

* Still trying pandoc

* Still trying pandoc

* Still trying pandoc (typo)

* Added latex

* Debugging missing extra packages

* Moved lfe install to test script

* Moved lfe install to test script (debugging)

* Added testthat

* Added haven for some reason

* Gave up and added lfe to recomended packages (for test)

* #42 Fxied line lengths

* #42 Shortened various lines; changed Delta^{XX} -> DeltaXX

* #42 changed Delta^{...} -> \eqn{\Delta^{...}}

* #42 Bumped version; fix example bug

* #42 Bumped version; fix example bug; fix .Rbuildignore

* #42 Removed inherit params from honest_did
mcaceresb added a commit that referenced this pull request May 11, 2024
@mcaceresb
Pull Request follow-up for #30: Example and Vignette runtime (#56)
fdadae3 
* Added basic tests

* Fixed minor typo; fixed tests

* Added pandoc to gh actions

* Still trying pandoc

* Still trying pandoc

* Still trying pandoc (typo)

* Added latex

* Debugging missing extra packages

* Moved lfe install to test script

* Moved lfe install to test script (debugging)

* Added testthat

* Added haven for some reason

* Gave up and added lfe to recomended packages (for test)

* Set long examples to donotrun; saved pre-computed vignette data

* Remove precompute from R build

* Tests only run conditional on environment variable

* Fix gh workflow typo

* Added missing VignetteResults.rdafile

* Fixed astray math in rd file

* Run tests only on linux

* Added MacOSX tests

* Debugging OSX

* Debugging OSX v2

* Debugging OSX v3

* Added title to sunab fun description
mcaceresb added a commit that referenced this pull request May 21, 2024
@mcaceresb
Pull Request follow-up for #30: Delete all Vignette References (#57)
0bbadb9 
* Added basic tests

* Fixed minor typo; fixed tests

* Added pandoc to gh actions

* Still trying pandoc

* Still trying pandoc

* Still trying pandoc (typo)

* Added latex

* Debugging missing extra packages

* Moved lfe install to test script

* Moved lfe install to test script (debugging)

* Added testthat

* Added haven for some reason

* Gave up and added lfe to recomended packages (for test)

* Deleted all Vignette references

* Link old vignette from README
mcaceresb added a commit that referenced this pull request Jun 11, 2024
@mcaceresb
Pull Request follow-up for #30: Minor formatting (#58)
618494f 
* Added basic tests

* Fixed minor typo; fixed tests

* Added pandoc to gh actions

* Still trying pandoc

* Still trying pandoc

* Still trying pandoc (typo)

* Added latex

* Debugging missing extra packages

* Moved lfe install to test script

* Moved lfe install to test script (debugging)

* Added testthat

* Added haven for some reason

* Gave up and added lfe to recomended packages (for test)

* Formatted citation -> added DOI; dontrun -> donttest
mcaceresb added a commit that referenced this pull request Jul 11, 2024
@mcaceresb
Pull Request follow-up for #30: Reset radom seed (#59)
e304b8e 
* Added basic tests

* Fixed minor typo; fixed tests

* Added pandoc to gh actions

* Still trying pandoc

* Still trying pandoc

* Still trying pandoc (typo)

* Added latex

* Debugging missing extra packages

* Moved lfe install to test script

* Moved lfe install to test script (debugging)

* Added testthat

* Added haven for some reason

* Gave up and added lfe to recomended packages (for test)

* Reset random seed, if it exists, after createSensitivityResults*()

* Make internal seed user-accessible

* Added seed option to function docs

* Added seed option to function docs (v2)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mcaceresb @jonathandroth @asheshrambachan