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

Attempt to use namespaces for stan_lmer and survreg #363

Merged
merged 10 commits into from
Apr 14, 2023

Conversation

rempsyc
Copy link
Member

@rempsyc rempsyc commented Apr 13, 2023

Attempt to use namespaces for stan_lmer and survreg. As showed in easystats/datawizard#401, works in reprex locally and console, but not in tests, so testing here to see if it fixes itself.

@IndrajeetPatil
Copy link
Member

I think you will also need to use GitHub version of datawizard for this to work.

DESCRIPTION Outdated Show resolved Hide resolved
rempsyc and others added 2 commits April 13, 2023 13:13
@codecov-commenter
Copy link

codecov-commenter commented Apr 13, 2023

Codecov Report

Merging #363 (6267d57) into main (4f8a9d1) will increase coverage by 6.40%.
The diff coverage is 87.01%.

❗ Current head 6267d57 differs from pull request most recent head eee8c2b. Consider uploading reports for the commit eee8c2b to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main     #363      +/-   ##
==========================================
+ Coverage   60.92%   67.32%   +6.40%     
==========================================
  Files          46       46              
  Lines        3173     3244      +71     
==========================================
+ Hits         1933     2184     +251     
+ Misses       1240     1060     -180     
Impacted Files Coverage Δ
R/report_table.R 53.84% <ø> (ø)
R/report_sample.R 88.38% <87.01%> (-0.60%) ⬇️

... and 12 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@rempsyc
Copy link
Member Author

rempsyc commented Apr 13, 2023

This test works locally:

x1 <- lm(Sepal.Length ~ Petal.Length * Species, data = iris)
testthat::expect_identical(
  as.character(report::report_performance(x1)),
  paste(
    "The model explains a statistically significant and substantial proportion of",
    "variance (R2 = 0.84, F(5, 144) = 151.71, p < .001, adj. R2 = 0.83)"
  )
)

Created on 2023-04-13 with reprex v2.0.2

But not using the random order test script in the console:

> purrr::walk(randomized_test_script_paths, test_path)
✔ All tests passing in `tests/testthat/test-report.data.frame.R`.All tests passing in `tests/testthat/test-report.lm.R`.All tests passing in `tests/testthat/test-format_model.R`.There was error while running tests in `tests/testthat/test-report_performance.R`.
Test `report_performance` has error:
expectation_success: as.character(report_performance(x1)) (`actual`) not identical to paste(...) (`expected`).

Even after renaming from x to x1 (there is no other object called x1). The tests are also fully contained within a testthat call. So no idea what the issue is?

@rempsyc
Copy link
Member Author

rempsyc commented Apr 14, 2023

I am not able to update snapshots for file test-report_performance.R. When running the tests locally in console with datawizard version ‘0.7.1.2’ I get no errors. But when using testthat::test_file("tests/testthat/test-report_performance.R"), I still get the following:

> testthat::test_file("tests/testthat/test-report_performance.R")
[ FAIL 2 | WARN 0 | SKIP 0 | PASS 12 ]

── Error (test-report_performance.R:109): report_performance Bayesian) ─────────
`report_performance(x7)` threw an unexpected error.
Message: Unable to refit the model with standardized data.
  Try instead to standardize the data (standardize(data)) and refit the
  model manually.
Class:   simpleError/error/condition

── Error (test-report_performance.R:113): report_performance Bayesian) ─────────
`summary(report_performance(x7))` threw an unexpected error.
Message: Unable to refit the model with standardized data.
  Try instead to standardize the data (standardize(data)) and refit the
  model manually.
Class:   simpleError/error/condition

[ FAIL 2 | WARN 0 | SKIP 0 | PASS 12 ]

Same behaviour when using RStudio's "Run Tests" or "Test" buttons.

@etiennebacher any idea? Namespace conflicts is your area of expertise now no? :P

@rempsyc
Copy link
Member Author

rempsyc commented Apr 14, 2023

Actually we can see the Unable to refit the model with standardized data. Try instead to standardize the data standardize(data)) and refit the model manually error in R CMD check here: https://github.com/easystats/report/actions/runs/4695121238/jobs/8323971085?pr=363#step:9:172

@etiennebacher
Copy link
Member

It's hard to explain but I think when rstanarm::stan_lmer is run interactively, it finds stan_glmer <- rstanarm::stan_glmer in the environment but when it runs in a new env generated by testthat it doesn't find it anymore.

One solution is to use stan_glmer <<- rstanarm::stan_glmer instead so that it attaches it to the global env. To avoid side effects you can add remove(stan_glmer, envir = .GlobalEnv) just before unloadNamespace("rstanarm")

@rempsyc
Copy link
Member Author

rempsyc commented Apr 14, 2023

Interesting! I don't fully get it but it seems to work, thanks! :D

@IndrajeetPatil IndrajeetPatil self-requested a review April 14, 2023 18:19
@IndrajeetPatil
Copy link
Member

@rempsyc Can you please also have a look at the test failure in random order? Also, test coverage workflow is failing.

@rempsyc
Copy link
Member Author

rempsyc commented Apr 14, 2023

All tests are finally passing, yeah! :-)

Copy link
Member

@IndrajeetPatil IndrajeetPatil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work, Remi! Thanks 😊

@IndrajeetPatil IndrajeetPatil merged commit e25ef38 into main Apr 14, 2023
@IndrajeetPatil IndrajeetPatil deleted the stan_glmer_test branch April 14, 2023 19:52
@rempsyc
Copy link
Member Author

rempsyc commented May 7, 2023

@etiennebacher the stan_glmer <<- rstanarm::stan_glmer workaround was working when we merged this, but now this is throwing this error both locally and on GAs:

Error in `eval(code, test_env)`: cannot change value of locked binding for 'stan_glmer'

A brief online search seems to confirm that this error is generated when using super assignment. In fact, it seems that this error arises when rstanarm is loaded first.

suppressPackageStartupMessages(library(rstanarm))

stan_glmer <<- rstanarm::stan_glmer
#> Error in eval(expr, envir, enclos): cannot change value of locked binding for 'stan_glmer'

Created on 2023-05-07 with reprex v2.0.2

Although we don't load it explicitly, sometimes, but not all the time (because this is rstanarm.....), it will load by itself just through using namespace.

Any idea for the best way to fix this? I also opened an issue at stan-dev/rstanarm#589

@etiennebacher
Copy link
Member

[I posted this by mistake in the rstanarm issue, so I deleted it there and put it here]

This is very well explained in this SO answer. Paraphrasing what is said there, the problem is that there's already a stan_glmer() loaded by rstanarm. Our <<- assignment found that, tried to change its value, and refused (because the function stan_glmer() is "locked", i.e. immutable).

I think one solution would be to check whether stan_glmer() is available and if it's not, use <<-. In the last test of test-report_performance, you could replace the <<- by

is_stan_glmer_avail <- !inherits(try(stan_glmer, silent = TRUE), "try-error")
if (!is_stan_glmer_avail) {
  stan_glmer <<- rstanarm::stan_glmer
  on.exit(remove(stan_glmer, envir = .GlobalEnv))
}

(don't forget to remove the remove() line at the end of the test).

Can you check if this works? It works for me locally but I couldn't reproduce the error when running devtools::test() in the first hand so who knows

@rempsyc
Copy link
Member Author

rempsyc commented May 13, 2023

Thanks @etiennebacher that seems to have solved the issue!! :)

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