search

metrumresearchgroup / bbr

R interface for model and project management
https://metrumresearchgroup.github.io/bbr/
Other
23 stars 2 forks source link

test-threads refactor #600

Closed barrettk closed 1 year ago

barrettk commented 1 year ago

addresses one of the bullets in https://github.com/metrumresearchgroup/bbr/issues/599

kyleam commented 1 year ago

@kyleam This PR is ready to review with the exception of being able to fetch nmrec and require a minimum version in the DESCRIPTION file

Okay, thanks. I'm working on $PROB support for nmrec. That shouldn't take long, so I'd like to get that into the next nmrec release.

Two initial thoughts :

Also, if you'd like to get a passing drone before the final adjustments, you can temporarily add something like

  - git clone --depth 1 https://github.com/metrumresearchgroup/nmrec.git /tmp/nmrec
  - R -s -e 'devtools::install("/tmp/nmrec", upgrade = "never")'

to the drone install steps.

barrettk commented 1 year ago
  • Using nmrec in this case can essentially be viewed as a fix. It's a good thing if the current tests pass for you, but shouldn't this also add regression cases for at least a few examples that were broken with the old code?

I initially didn't think there were any cases where the old code would have broken, but after thinking on it a bit there definitely were - mainly models that used one of the weird spellings of maxevals or something. We captured all the frequent cases with regex, but definitely did not support all of them (dont think we supported lower case either). Can add a test for this type of thing unless there were any other cases you had in mind. Im changing some of the tests now to rely on nmrec as well, so our existing tests will be a bit better.

  • Before this is merged, we should discuss (offline) how to handle nmrec as a dependency: Refactor current functions to use nmrec #599 (comment). It's probably best to mark this as a draft until then, even when it's ready for a full review. (What we decide in that discussion main mean that more than an nmrec release would block the merge here.)

Yeah this sounds good. As an aside, my intention was just to address the test-threads changes in this PR, but we could broaden the scope to include all the changes mentioned in that issue as well.

Also, if you'd like to get a passing drone before the final adjustments, you can temporarily add something like

- git clone --depth 1 https://github.com/metrumresearchgroup/nmrec.git /tmp/nmrec
- R -s -e 'devtools::install("/tmp/nmrec", upgrade = "never")'

Thanks, will try this out once I fix the tests

barrettk commented 1 year ago

@kyleam FYI it seems like it's failing solely because of gregexec in nmrec:::split_to_elements (R CMD check and devtools::test() pass locally), which is not included in R 4.0. I have reported a similar issue in the past for an MPN package, so im wondering if we want to make a fix for this on the nmrec side.

As an aside, we may want to add R 4.0 to drone in nmrec before the first official release

kyleam commented 1 year ago

FYI it seems like it's failing solely because of gregexec in nmrec:::split_to_elements

Thanks, didn't realize that was a recent addition. Fixed in https://github.com/metrumresearchgroup/nmrec/pull/11

kyleam commented 1 year ago

We captured all the frequent cases with regex, but definitely did not support all of them (dont think we supported lower case either).

Hmm, I'm not as confident as you are about what's frequent, but, yeah, the idea is that, by using nmrec, you support all the spellings, as well as little things that the previous bbr code doesn't consider. For example, the option name could be NBURN = 1000 or even NBURN 1000.

Can add a test for this type of thing unless there were any other cases you had in mind.

Thanks. I don't care about any particular case (and I don't think it's necessary to exhaustively cover weird cases; that's nmrec's job) but having a few cases that wouldn't work before this change is a good thing in my view.

As an aside, my intention was just to address the test-threads changes in this PR, but we could broaden the scope to include all the changes mentioned in that issue as well.

My vote is to keep this PR focused on test-threads.

barrettk commented 1 year ago

@kyleam got drone to work by re-running it. Seems like we will wait to actually merge this until nmrec is officially released, but figured i'd still ask for a review now (no rush).

kyleam commented 1 year ago

requested a review from kyleam now

I've already approved. Is there something in particular you want me to look at? If not, merge if you're happy with its state.

barrettk commented 1 year ago

I've already approved. Is there something in particular you want me to look at? If not, merge if you're happy with its state.

Yeah fair - I made some changes based on some of your comments, and just wanted to make sure you were good with the changes. Will merge now