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 submission #818

Merged
merged 8 commits into from
Oct 12, 2023
Merged

CRAN submission #818

merged 8 commits into from
Oct 12, 2023

Conversation

strengejacke
Copy link
Member

No description provided.

@codecov
Copy link

codecov bot commented Oct 8, 2023

Codecov Report

Merging #818 (fa80ef0) into main (89b7798) will increase coverage by 0.10%.
The diff coverage is n/a.

❗ Current head fa80ef0 differs from pull request most recent head 151689f. Consider uploading reports for the commit 151689f to get more accurate results

@@            Coverage Diff             @@
##             main     #818      +/-   ##
==========================================
+ Coverage   54.76%   54.86%   +0.10%     
==========================================
  Files         124      124              
  Lines       15337    15337              
==========================================
+ Hits         8399     8415      +16     
+ Misses       6938     6922      -16     
Files Coverage Δ
R/clean_parameters.R 64.46% <ø> (ø)
R/find_formula.R 66.53% <ø> (ø)
R/find_parameters.R 31.97% <ø> (ø)
R/find_parameters_bayesian.R 67.91% <ø> (ø)
R/find_parameters_mfx.R 0.00% <ø> (ø)
R/format_table.R 59.95% <ø> (ø)
R/get_auxiliary.R 39.47% <ø> (ø)
R/get_loglikelihood.R 65.15% <ø> (ø)
R/get_parameters.R 38.41% <ø> (ø)
R/get_parameters_bayesian.R 36.82% <ø> (ø)
... and 9 more

... and 1 file with indirect coverage changes

@strengejacke
Copy link
Member Author

strengejacke commented Oct 9, 2023

@easystats/core-team I have to update insight due to changes in glmmTMB that break our tests (see #806), however, there are some strange issues that I get from winbuilder (only R devel)... These issues rather look like false positives, i.e. not related to insight? Some failures are related to glmmTMB, but then I wonder why we don't see these issues in their tests (https://cran.r-project.org/web/checks/check_results_glmmTMB.html)? (unless this is not tested...)

https://win-builder.r-project.org/aa3Mx7oHVDG0/00check.log

also tagging @bbolker and @mebrooks

@bbolker
Copy link
Contributor

bbolker commented Oct 9, 2023

Hmm, we certainly can't test everything, but to pick one example out at random (the logs are a little overwhelming), I have no problem with this one:

Error ('test-model_info.R:44:3'): glmmTMB bernoulli ─────────────────────────
  Error in `fitTMB(TMBStruc)`: negative log-likelihood is NaN at starting parameter values
  Backtrace:
      ▆
   1. └─glmmTMB::glmmTMB(vs ~ disp + (1 | cyl), data = mtcars, family = binomial()) at test-model_info.R:44:2
   2.   └─glmmTMB::fitTMB(TMBStruc)

Whereas locally (with R Under development (unstable) (2023-09-20 r85179), glmmTMB_1.1.8-9000) it seems fine.

glmmTMB::glmmTMB(vs ~ disp + (1 | cyl), data = mtcars, family = binomial())
Formula:          vs ~ disp + (1 | cyl)
Data: mtcars
      AIC       BIC    logLik  df.resid 
 28.69583  33.09304 -11.34792        29 
## etc.

I see lots and lots of Matrix-related errors. Maybe see what's going on with that first ... ?

@strengejacke
Copy link
Member Author

I see lots and lots of Matrix-related errors. Maybe see what's going on with that first ... ?

Yes, there are some errors that seem related to the Matrix package, like

── Error ('test-cpglmm.R:9:1'): (code run outside of `test_that()`) ────────────
  Error in `mkZt(FL, NULL)`: function 'as_cholmod_sparse' not provided by package 'Matrix'

glmmTMB errors, however, also show issues with glmmTMB::fitTMB(TMBStruc)?

  ── Error ('test-model_info.R:44:3'): glmmTMB bernoulli ─────────────────────────
  Error in `fitTMB(TMBStruc)`: negative log-likelihood is NaN at starting parameter values
  Backtrace:1. └─glmmTMB::glmmTMB(vs ~ disp + (1 | cyl), data = mtcars, family = binomial()) at test-model_info.R:44:2
   2.   └─glmmTMB::fitTMB(TMBStruc)

@bbolker
Copy link
Contributor

bbolker commented Oct 10, 2023

I saw those fitTMB() errors. There's nothing I can think of off the top of my head that we caused that would cause these problems - I will take a quick look to see if I can diagnose, but I was suggesting that you resolve the Matrix problems first on the off chance that the fitTMB() errors are actually downstream of them ...

  • it's still confusing to me why your tests relying on glmmTMB are only breaking now. Shouldn't you have needed to load glmmTMB, or use glmmTMB::glmmTMB(), all along?
  • It is of course conceivable that some combination of subtle changes (in code and in compiler etc.) could mess with a numerically unstable fit so that it worked on some platforms but not others, but these don't seem to be particularly delicate fits? But at least the one fit I can see without digging into the testthat files by line number (glmmTMB::glmmTMB(mpg ~ disp + (1 | cyl) + offset(log(wt)), data = mtcars)) doesn't seem to be particular crazy?
  • FWIW, when I run devtools::test() locally I get none of the errors listed above, this one instead ...
── Failed tests ───────────────────────────────────────
Error (test-get_variance.R:371:3): fixed effects variance for rank-deficient models, #765
Error: argument `expected` is missing, with no default.
Backtrace:
    ▆
 1. └─testthat::expect_equal(out, var.fixed = 627.03661, tolerance = 1e-04) at test-get_variance.R:371:2
 2.   └─testthat::quasi_label(enquo(expected), expected.label, arg = "expected")

@strengejacke
Copy link
Member Author

I wonder why the Matrix package has no failures on the CRAN page:
https://cran.r-project.org/web/checks/check_results_Matrix.html

This seems like it's in general a false positive from win-builder - I don't think there are real issues with glmmTMB. Furthermore, win-devel on the CRAN page also shows none of the errors from the win-builder log:
https://www.r-project.org/nosvn/R.check/r-devel-windows-x86_64/insight-00check.html

Maybe I just ignore the win-builder (only R-devel errors) results for now and try to submit. All other checks, including revdep, passed.

FWIW, when I run devtools::test() locally I get none of the errors listed above, this one instead ...

That should be fixed here: e913fd4

@bbolker
Copy link
Contributor

bbolker commented Oct 10, 2023

I agree that these are probably false positives. If it were me I would check on r-hub's Windows platforms; if it's OK there then I would submit with a note explaining the issue.

@bwiernik
Copy link
Contributor

I rarely use win-builder anymore and just just r-hub's windows servers. I've run into a lot fewer issues like these

@strengejacke
Copy link
Member Author

I've never used rhub before 😬 But I just uploaded to rhub, Win-devel, waiting for the result now...

@strengejacke
Copy link
Member Author

@bwiernik
Copy link
Contributor

Rhub basically runs the CRAN checks with good fidelity--I say it completely replaces the role of win-builder

@IndrajeetPatil
Copy link
Member

Hmm, I'm wondering why the GHA windows-devel workflow doesn't catch the same failures as R-hub. What is R-hub doing differently? Maybe we should also adapt the GHA workflow to align with what R-hub is doing.

@bbolker
Copy link
Contributor

bbolker commented Oct 11, 2023

I have to say that chasing obscure, heterogeneous test failures across multiple platforms is (by far) my least favourite part of package development ... All three of the windows platforms are reporting different errors ...

@strengejacke
Copy link
Member Author

Ok, obviously, CRAN checks work slightly different. None of the issues reported by win builder or rhub were mentioned during submission...

@strengejacke strengejacke merged commit 95ebd50 into main Oct 12, 2023
23 of 25 checks passed
@strengejacke strengejacke deleted the rc_0_19_6 branch October 12, 2023 11:48
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

Successfully merging this pull request may close these issues.

4 participants