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

Pull Request follow-up for #30: Various minor fixes #42

Merged
merged 10 commits into from
Oct 22, 2023
Merged

Pull Request follow-up for #30: Various minor fixes #42

merged 10 commits into from
Oct 22, 2023

Conversation

mcaceresb
Copy link
Collaborator

@mcaceresb mcaceresb commented Sep 12, 2023 •
edited
Loading

This addresses 5 of the 6 requests here.

Done:

  • Replaced author and maintainer fields with Authors@R; gave both Ashesh and Jon author and copyright and Ashesh creator/maintainer
  • Removed all standardtext from .Rd files (e.g. placeholder comments in main/basisVector.R)
  • Changed all T/F to TRUE/FALSE
  • Closed [ in DESCRIPTION
  • Added return value to man/constructOriginalCS.Rd

Pending:

  • Add executable examples to each doc file; e.g.
\examples{
  foo(bar)
}

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

@asheshrambachan @jonathandroth I added a very simple example for several functions (though not stuff that seemed to mainly be used internally like computeConditionalCS*).

I figured it'd be redundant to make longer examples in light of the vignette, so I wrote a comment in each directing people there. If that sounds fine then I can merge into master and you can try again.

@jonathandroth
Copy link
Collaborator

jonathandroth commented Sep 28, 2023 via email

@mcaceresb
Copy link
Collaborator Author

@jonathandroth I changed all of them so they reference the name of the vignette, so they all prompt the user to call vignette("HonestDiD_Example")

@jonathandroth
Copy link
Collaborator

jonathandroth commented Oct 2, 2023 via email

@mcaceresb
Copy link
Collaborator Author

@jonathandroth It opens the vignette. For pdf documents it opens it in the pdf viewer specified by getOption("pdfviewer"); for html documents it opens it in the HTML viewer specified by getOption("browser"). Not sure if pdfviewer and browser are set by default (they weren't for me and I had to set them manually in my console; though the default HTML viewer in R-studio is just it's side pane, which is the one used to open html help for regular functions).

I've found references to vignette("name") in several packages so I figured it was convention. LMK if it sounds OK. (Though I get the impression referencing HTML vignettes is more common; maybe because people don't have to set an HTML viewer in an IDE but they have to set a pdfviewer either way?)

@jonathandroth
Copy link
Collaborator

jonathandroth commented Oct 2, 2023 via email

@mcaceresb
Copy link
Collaborator Author

@jonathandroth How about

for more detailed examples, see <https://github.com/asheshrambachan/HonestDiD>

?

@jonathandroth
Copy link
Collaborator

jonathandroth commented Oct 16, 2023 via email

@mcaceresb mcaceresb merged commit 10c13d6 into asheshrambachan:master Oct 22, 2023
@mcaceresb mcaceresb deleted the cran-commit-round4 branch October 22, 2023 19:48
@mcaceresb
Copy link
Collaborator Author

@jonathandroth @asheshrambachan I think we're good to try again. All the points here were addressed. Apologies for the delay.

@jonathandroth
Copy link
Collaborator

Thanks @mcaceresb ! @asheshrambachan, let us know when you hear back from CRAN!

@jonathandroth
Copy link
Collaborator

@asheshrambachan any luck submitting this to CRAN ?

@mcaceresb
Copy link
Collaborator Author

@jonathandroth @asheshrambachan Checking in on the CRAN status?

@asheshrambachan
Copy link
Owner

Resubmitted to CRAN today. I'll let you know if we get back any things to fix.
Screenshot 2024-02-23 at 3 52 47 PM

@jonathandroth
Copy link
Collaborator

jonathandroth commented Feb 23, 2024 via email

@asheshrambachan
Copy link
Owner

asheshrambachan commented Feb 24, 2024 •
edited
Loading

We received the following response on the CRAN submission. It looks like overall some issues with the documentation, and we have to fix them before resubmitting.
CRAN_Submission.pdf

@mcaceresb
Copy link
Collaborator Author

@asheshrambachan @jonathandroth On the five notes:

  1. Rambachan and RESTUD are not misspelled, so ignore.
  2. .github shouldn't be included in the final folder. Was this my error? LMK where you got the file you uploaded.
  3. There's a set of notes noting Delta^{SD}(M) and similar LaTeX-style parts of the descriptions need an escape. I'll look around about how to do that because I'm not sure.
  4. Some lines are too long; fixed.
  5. Apparently some examples take a while to execute, but that's just how the function is. I could make the grid small for the examples, though? LMK.

mcaceresb added a commit to mcaceresb/HonestDiD that referenced this pull request Feb 25, 2024
@mcaceresb
asheshrambachan#42 Fxied line lengths
c3764a0
@jonathandroth
Copy link
Collaborator

jonathandroth commented May 21, 2024 via email

@mcaceresb
Copy link
Collaborator Author

@jonathandroth I'm deleting this from the README, then?

See the previous package [vignette] for additional examples and package options, including incorporating sign and monotonicity restrictions, and combining relative magnitudes and smoothness restrictions.

?

@mcaceresb mcaceresb mentioned this pull request May 21, 2024
Merged
@jonathandroth
Copy link
Collaborator

jonathandroth commented May 21, 2024 via email

@mcaceresb
Copy link
Collaborator Author

@jonathandroth Like this?

@jonathandroth
Copy link
Collaborator

jonathandroth commented May 21, 2024 via email

@mcaceresb
Copy link
Collaborator Author

@asheshrambachan Can you try again?

@asheshrambachan
Copy link
Owner

Sorry for the delay. Just submitted the updated version to CRAN (see attached screenshot). I'll let you know what happens.

Screenshot 2024-06-09 at 8 49 23 PM

@jonathandroth
Copy link
Collaborator

jonathandroth commented Jun 10, 2024 via email

@asheshrambachan
Copy link
Owner

Good news: HonestDiD passed all of the incoming checks! Thanks @mcaceresb for all the work to clean up the package.

There are a few minor style comments the CRAN managers had for us -- see the screenshot below. My understanding is: since we passed the automatic incoming checks, these style points are the last hurdle to CRAN.

Screenshot 2024-06-11 at 8 47 43 AM

@jonathandroth
Copy link
Collaborator

jonathandroth commented Jun 11, 2024 via email

@mcaceresb
Copy link
Collaborator Author

@asheshrambachan @jonathandroth They literally told us our examples took too long to run... I had an entire internal debate about using dontrun vs donttest and I decided to do the former to be cautious just in case, and it seems I erred on the wrong side of caution.

Anyway, made both changes.

@mcaceresb mcaceresb mentioned this pull request Jun 11, 2024
Merged
@asheshrambachan
Copy link
Owner

Thanks @mcaceresb! Just resubmitted. Fingers crossed this time.
Screenshot 2024-06-11 at 3 01 09 PM

@jonathandroth
Copy link
Collaborator

jonathandroth commented Jun 11, 2024 via email

@mcaceresb
Copy link
Collaborator Author

@jonathandroth @asheshrambachan No, I missed it! I'm sorry. I'll see what that's about as soon as I can. Really sorry!

@jonathandroth
Copy link
Collaborator

jonathandroth commented Jun 11, 2024 via email

@jonathandroth
Copy link
Collaborator

jonathandroth commented Jul 1, 2024 via email

@asheshrambachan
Copy link
Owner

Have we converged on what to do about setting the seed per the feedback from CRAN? I see the pull request there is still open, so I have been holding off on resubmitting until that's resolved.

@jonathandroth
Copy link
Collaborator

@asheshrambachan I'm slightly confused because it looked like you said you resubmitted it already (before I brought up the issue with the seed)? You posted a screenshot earlier in this thread, so I was wondering if they responded to that.

@mcaceresb -- what is the latest with handling the seed?

@asheshrambachan
Copy link
Owner

@jonathandroth Ah yup -- they flagged the same concern about the seed, and so I have not resubmitted it since then. Sorry for the miscommunication!

@jonathandroth
Copy link
Collaborator

No worries! Well at least it sounds like they didn't complain about anything else?

@asheshrambachan
Copy link
Owner

Correct! The package otherwise passed all checks, so (fingers crossed) once this is fixed we should be good to go.

@mcaceresb mcaceresb mentioned this pull request Jul 10, 2024
Merged
@mcaceresb
Copy link
Collaborator Author

This has been addressed after the merge from #59

@asheshrambachan @jonathandroth

@jonathandroth
Copy link
Collaborator

Thanks, Mauricio! @asheshrambachan you should be good to resubmit!

@asheshrambachan
Copy link
Owner

Thanks @mcaceresb! Just resubmitted the package.

Screenshot 2024-07-11 at 5 22 58 PM

@jonathandroth
Copy link
Collaborator

jonathandroth commented Jul 12, 2024 via email

@asheshrambachan
Copy link
Owner

Good news! Package looks to be on its way to CRAN.

Screenshot 2024-07-14 at 10 18 39 AM

@jonathandroth
Copy link
Collaborator

jonathandroth commented Jul 15, 2024 via email

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