search

ropensci / GLMMcosinor

An R package for flexible cosinor modelling using the glmmTMB framework
https://docs.ropensci.org/GLMMcosinor/
GNU General Public License v3.0
1 stars 0 forks source link

Dev -> main #17

Closed RWParsons closed 3 months ago

maelle commented 3 months ago

:wave: @RWParsons any update? any help needed?

RWParsons commented 3 months ago

Hi @maelle,

Yeah I'm not sure what's causing the check failures here. It works on my local windows machine but I'm struggling to recreate the issues that GHA is having.

If you could help or know someone that could then that would be perfect! I'm going through a very busy period at the moment and for the next month or so, so am struggling to make time to work on this.

Thanks!

maelle commented 3 months ago

Why use renv inside the package project? I am used to getting the newest dependencies whenever I start working on a package, by running pak::pak().

maelle commented 3 months ago

Another suggestion (since I'm looking while I wait for the dependencies to update :grin:): inside tests rather than have everything nested within withr::with_seed(), why not just use withr::local_seed()? It will only apply to the test_that() call it is in, and will make the test easier to read.

Rather than "test 7" etc I'd recommend splitting the test_that() into several test_that() with informative names.

maelle commented 3 months ago

Locally I do get the snapshot failures. Did the vitamind data change? Or could it be due to an update to one of the dependencies? Here I am lacking domain expertise.

Note that I get the snapshot failures for the main branch too, not only the dev branch.

maelle commented 3 months ago

We can have a debugging call if needed once you're less busy. :slightly_smiling_face:

RWParsons commented 3 months ago

Hi @maelle - thanks so much for the investigating!

Why use renv inside the package project? I am used to getting the newest dependencies whenever I start working on a package, by running pak::pak().

Yeah I agree that renv isn't the best solution. It was a quick solution though to ensure that I have compatible versions of TMB and Matrix (see the section here: https://glmmtmb.github.io/glmmTMB/).

Another suggestion (since I'm looking while I wait for the dependencies to update 😁): inside tests rather than have everything nested within withr::with_seed(), why not just use withr::local_seed()? It will only apply to the test_that() call it is in, and will make the test easier to read. Rather than "test 7" etc I'd recommend splitting the test_that() into several test_that() with informative names.

Yeah sounds like a good move!

Locally I do get the snapshot failures. Did the vitamind data change? Or could it be due to an update to one of the dependencies? Here I am lacking domain expertise. Note that I get the snapshot failures for the main branch too, not only the dev branch.

I think I changed the vitamind data very early on so perhaps (but I doubt it) - I changed the column names but none of the data.

maelle commented 3 months ago

all green :clap: :tada:

RWParsons commented 3 months ago

Thanks, @maelle for all the helpful suggestions and prompting on this. I managed to sort out my GHA and have (mostly) cleaned up the tests as you suggested. I removed renv and it seems to be working on GHA now. (Hopefully, it stays that way and I don't get problems with Matrix/TMB/glmmTMB in the near future!)

🚀 🚀 🚀

maelle commented 3 months ago

Awesome to read, well done :clap: