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.

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

CRAN release #165

Closed
jgabry opened this issue Jun 23, 2021 · 34 comments
Closed

CRAN release #165

jgabry opened this issue Jun 23, 2021 · 34 comments

Comments

@jgabry
Copy link
Member

jgabry commented Jun 23, 2021

I think posterior is getting close to the point where we should consider releasing the first CRAN version. I'm opening this issue so we have a place to discuss the things we need/want to get done before then.

@mjskay
Copy link
Collaborator

mjskay commented Jun 24, 2021

I am 100% on board with this, as I already have feature branches of ggdist and tidybayes that I can't submit to cran because they depend on posterior :).

Is it worth asking: is there anything missing from the CRAN milestone? Or conversely, anything on there that isn't necessary for the first CRAN release? e.g. nice-to-haves that wouldn't change the API in a backwards-incompatible way.

@paul-buerkner
Copy link
Collaborator

I agree. I think one minor thing would be to decide what to do with the iter argument in subset_draws.draws_matrix if we want to add chain information to the format later on. My guess would be that it's best right now to disallow this argument for now and require users to always subset by draw which will remain valid even after adding chain information.

With regard to the CRAN milestone, I think issues #42, #77, and #105 could be dropped from the milestone and also handled later. The other issues we could try to fix more or less right away when somebody has time.

@jgabry
Copy link
Member Author

jgabry commented Jun 24, 2021

Is it worth asking: is there anything missing from the CRAN milestone?

I think we should decide whether to include chain info in draws_matrix. But I'm a bit biased because my motivation for that is the CmdStanR issue related to losing chain info (stan-dev/cmdstanr#523). I'm more eager than I used to be to decide one way or another on this issue so that we know if we need to implement a somewhat hacky solution in CmdStanR or not. Are you all ok with deciding on this before the CRAN release?

With regard to the CRAN milestone, I think issues #42, #77, and #105 could be dropped from the milestone and also handled later.

Agreed. I just went ahead removed them from the milestone (but if anyone disagrees we can add them back).

@paul-buerkner
Copy link
Collaborator

I agree, we should probably add chain info to the draws_matrix format. There are apparently enough use cases for this, to make it worthwhile implementing and maintaining.

@paul-buerkner paul-buerkner modified the milestone: CRAN release Jun 30, 2021
@paul-buerkner
Copy link
Collaborator

With all issues of the CRAN milestone now closed, I think we can actually move towards the submission.

@jgabry @mjskay @avehtari since you are authors of this package, I would like to get your explicit approval for the CRAN submission.

Asking anybody reading this, is there anything that you think needs to be added to the first CRAN release? Specfically things that are much harder to change once things are on CRAN?

@jgabry
Copy link
Member Author

jgabry commented Jul 1, 2021

Thanks Paul.

I would like to get your explicit approval for the CRAN submission.

Approved!

@jgabry
Copy link
Member Author

jgabry commented Jul 1, 2021

Should we use 1.0 as the version number for the CRAN release?

@paul-buerkner
Copy link
Collaborator

Yes, I think we should move to 1.0 for the release.

@mjskay
Copy link
Collaborator

mjskay commented Jul 1, 2021

Approved as well, wholeheartedly!

@avehtari
Copy link
Collaborator

avehtari commented Jul 2, 2021

I've read this thread and checked some linked things and don't see or remember anything to comment.
I approve.

@paul-buerkner
Copy link
Collaborator

paul-buerkner commented Jul 2, 2021

Ok, great! I will go ahead and initial the CRAN release. I will let you know how it goes.

@jgabry
Copy link
Member Author

jgabry commented Jul 2, 2021

Thank you!

@paul-buerkner
Copy link
Collaborator

We got feedback from CRAN. Here is what we need to fix still:

Thanks, we see

License components with restrictions and base license permitting such:
BSD_3_clause + file LICENSE
File 'LICENSE':
YEAR: 2021
COPYRIGHT HOLDER: posterior package authors; Stan Developers and their Assignees; and Trustees of Columbia University

You should have in the template:

YEAR:
COPYRIGHT HOLDER:
ORGANIZATION:

If there are references describing the methods in your package, please add these in the description field of your DESCRIPTION file in the form
authors (year) doi:...
authors (year) arXiv:...
authors (year, ISBN:...)
or if those are not available: https:...
with no space after 'doi:', 'arXiv:', 'https:' and angle brackets for auto-linking.
(If you want to add a title as well please put it in quotes: "Title")

Please add \value to .Rd files regarding exported methods and explain the functions results in the documentation. Please write about the structure of the output (class) and also what the output means. (If a function does not return a value, please document that too, e.g. \value{No return value, called for side effects} or similar)
Missing Rd-tags:
diagnostics.Rd: \value
draws_of.Rd: \value
draws-index.Rd: \value
quantile2.Rd: \value
reserved-variables.Rd: \value
vctrs-compat.Rd: \value

\dontrun{} should only be used if the example really cannot be executed (e.g. because of missing additional software, missing API keys, ...) by the user. That's why wrapping examples in \dontrun{} adds the comment ('# Not run:') as a warning for the user.
Does not seem necessary.

Please unwrap the examples if they are executable in < 5 sec, or replace \dontrun{} with \donttest{}.

@mjskay
Copy link
Collaborator

mjskay commented Jul 5, 2021

I'm happy to quickly write @return for stuff, I will do that today.

I also have a fix for a minor corner case in rvar matrix multiplication I ran into yesterday that I am about to submit a PR for if it fits under the wire (nothing big --- base vectors weren't being promoted to row/col vectors, so some operations that should have worked instead returned an error, forcing the user to promote manually).

@mjskay
Copy link
Collaborator

mjskay commented Jul 5, 2021

On return values: How much of a fight with CRAN do we want to get into over reserved-variables.Rd? It's not even a function, just a list of variable names, so it would be a little silly to have a \value section on that page. Would a @docType directive fix the problem so that it is not flagged as a function needing return value documentation? Though I'm not sure what docType to assign it (can you have multiple pages with @docType package?). Alternatively we add a stub "return" that says something silly like "this is not a function" :)

@mjskay
Copy link
Collaborator

mjskay commented Jul 5, 2021

Re: reserved-variables, I see that the reserved_variables() generic is not exported. Could we export that? That might solve the problem, as then we can simply move that doc page to the page for reserved_variables. I will make a separate PR from other fixes with that proposal.

@mjskay
Copy link
Collaborator

mjskay commented Jul 5, 2021

I was feeling productive this afternoon so I proposed PRs for everything except the LICENSE and DESCRIPTION changes, which I wasn't sure how you wanted to handle.

@paul-buerkner
Copy link
Collaborator

Thank you for those PRs. Just just merged all of them.

With regard to the description, shall we just link to github? Or is there something else to link to? Shall we add a link at all? If not, I will add to the CRAN comments, that we don't have a reference (yet) and as such don't link to in the description.

With regard to the license, I have no idea what we shall put under "ORGANIZATION". @jgabry you seem to be more experienced with licensing. Do you have any recommendation?

@mjskay
Copy link
Collaborator

mjskay commented Jul 6, 2021

With regard to the description, shall we just link to github? Or is there something else to link to? Shall we add a link at all? If not, I will add to the CRAN comments, that we don't have a reference (yet) and as such don't link to in the description.

Maaaybe section (d) of description could cite convergence metrics papers? But also I think saying we don't have a reference yet makes sense.

@paul-buerkner
Copy link
Collaborator

Good idea, citing for example vehtari et al. 2020 for new Rhat, ESS etc. could make a lot of sense.

@paul-buerkner
Copy link
Collaborator

I now simply added a blank ORGANIZATION field to the description for now as I don't really see what we shall add there. @jgabry any objections to leaving this field blank?

@paul-buerkner
Copy link
Collaborator

Judging from the BSD3_clause CRAN template (https://cran.r-project.org/web/licenses/BSD_3_clause), it seems that they put <Organization> in the template where we would like to have copyright holder written. So how about simply using ORGANIZATION: copyright holder?

@mike-lawrence
Copy link

mike-lawrence commented Jul 7, 2021

My renv project seems to have automatically pulled the 1.0 version in which it seems that the ess_tail() function is yielding NA for even it's own example.

> sessionInfo()
R version 4.1.0 (2021-05-18)
Platform: x86_64-pc-linux-gnu (64-bit)
Running under: Ubuntu 20.04.2 LTS

Matrix products: default
BLAS:   /usr/lib/x86_64-linux-gnu/openblas-pthread/libblas.so.3
LAPACK: /usr/lib/x86_64-linux-gnu/openblas-pthread/liblapack.so.3

locale:
 [1] LC_CTYPE=en_CA.UTF-8       LC_NUMERIC=C              
 [3] LC_TIME=en_CA.UTF-8        LC_COLLATE=en_CA.UTF-8    
 [5] LC_MONETARY=en_CA.UTF-8    LC_MESSAGES=en_CA.UTF-8   
 [7] LC_PAPER=en_CA.UTF-8       LC_NAME=C                 
 [9] LC_ADDRESS=C               LC_TELEPHONE=C            
[11] LC_MEASUREMENT=en_CA.UTF-8 LC_IDENTIFICATION=C       

attached base packages:
[1] stats     graphics  grDevices datasets  utils     methods  
[7] base     

other attached packages:
[1] posterior_1.0.0 aria_0.1.0     

loaded via a namespace (and not attached):
 [1] matrixStats_0.58.0   fs_1.5.0             usethis_2.0.1       
 [4] lubridate_1.7.10     devtools_2.4.1       httr_1.4.2          
 [7] rprojroot_2.0.2      rstan_2.21.2         tensorA_0.36.2      
[10] tools_4.1.0          backports_1.2.1      utf8_1.2.1          
[13] R6_2.5.0             DBI_1.1.1            colorspace_2.0-1    
[16] withr_2.4.2          tidyselect_1.1.1     gridExtra_2.3       
[19] prettyunits_1.1.1    processx_3.5.2       curl_4.3.1          
[22] compiler_4.1.0       rvest_1.0.0          cli_2.5.0           
[25] RNetCDF_2.4-2        xml2_1.3.2           desc_1.3.0          
[28] stringfish_0.15.1    scales_1.1.1         checkmate_2.0.0     
[31] readr_1.4.0          callr_3.7.0          stringr_1.4.0       
[34] digest_0.6.27        StanHeaders_2.21.0-7 pkgconfig_2.0.3     
[37] sessioninfo_1.1.1    dbplyr_2.1.1         fastmap_1.1.0       
[40] readxl_1.3.1         rlang_0.4.11         rstudioapi_0.13     
[43] generics_0.1.0       farver_2.1.0         RApiSerialize_0.1.0 
[46] jsonlite_1.7.2       dplyr_1.0.6          distributional_0.2.2
[49] inline_0.3.18        magrittr_2.0.1       loo_2.4.1           
[52] Rcpp_1.0.6           munsell_0.5.0        fansi_0.5.0         
[55] abind_1.4-5          lifecycle_1.0.0      stringi_1.6.2       
[58] pkgbuild_1.2.0       grid_4.1.0           parallel_4.1.0      
[61] forcats_0.5.1        crayon_1.4.1         haven_2.4.1         
[64] hms_1.1.0            knitr_1.33           ps_1.6.0            
[67] pillar_1.6.1         codetools_0.2-18     stats4_4.1.0        
[70] pkgload_1.2.1        reprex_2.0.0         glue_1.4.2          
[73] beepr_1.3            V8_3.4.2             modelr_0.1.8        
[76] data.table_1.14.0    remotes_2.3.0        renv_0.13.2         
[79] RcppParallel_5.1.4   vctrs_0.3.8          cellranger_1.1.0    
[82] testthat_3.0.2       gtable_0.3.0         purrr_0.3.4         
[85] tidyr_1.1.3          qs_0.24.1            assertthat_0.2.1    
[88] cachem_1.0.5         ggplot2_3.3.3        xfun_0.23           
[91] broom_0.7.6          tidyverse_1.3.1      roxygen2_7.1.1      
[94] audio_0.1-7          tibble_3.1.2         memoise_2.0.0       
[97] ellipsis_0.3.2       cmdstanr_0.4.0.9000

posterior GithubSHA1: ee38b68

@paul-buerkner
Copy link
Collaborator

This is related to #167. Do you mind posting there again and we continue the discussion there?

@paul-buerkner
Copy link
Collaborator

paul-buerkner commented Jul 13, 2021

Since I haven't heard anything to the contrary I go ahead and make sure that the LICENSE implied license (according to the CRAN BSD3 template) and our LICENSE.md match.

I will go ahead and submit posterior to CRAN again today.

@jgabry
Copy link
Member Author

jgabry commented Jul 13, 2021 via email

@paul-buerkner
Copy link
Collaborator

posterior has been accepted by CRAN and should appear there soon. Very well done everyone! :-)

@mjskay
Copy link
Collaborator

mjskay commented Jul 13, 2021

Awesome! Very exciting :)

@jgabry
Copy link
Member Author

jgabry commented Jul 13, 2021 via email

@jgabry
Copy link
Member Author

jgabry commented Jul 13, 2021 via email

@jgabry
Copy link
Member Author

jgabry commented Jul 13, 2021 via email

@jgabry
Copy link
Member Author

jgabry commented Jul 13, 2021

Also, I won't have time to do it myself until next week (I'm teaching a Stan short course this week), but if anyone has time to write a blog post about the package you can send it to me and I can post it on Andrew's blog right away. Or I can try to write something up next week when I have time.

@paul-buerkner
Copy link
Collaborator

As a welcome from one of the CRAN members, we were immediately threatend that posterior will be removed from CRAN in two weeks notice if we don't fix one speed test that fails on some machines. :-D

I will skip speed tests on CRAN (via skip_on_cran) for this reason and we should remember also to skip future speed tests on CRAN.

I will handle the resubmission and also sequeeze in @mjskay's latest PR #179.

@jgabry
Copy link
Member Author

jgabry commented Jul 14, 2021 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants