Add support for etas attached to idata

[kylebaron]

Add on to #1037 ; etasrc can now be

• idata
• idata.all

Re-using all of the get_etas() code.

Checklist

• Update devtran to accept etasrc = "idata" or etasrc = "idata.all"
• Make sure get_etas() works with idata object; one minor change to fill out Startrow for idata sets.
• Add tests
• Documentation

Discovered a bug when implementing #1092 where ETA was missed if it was in the first column of the data set. Please see #1095 for the fix.

[kyleam]

Re-using all of the get_etas() code.

That worked out cleanly. Looks good to me.

[kyleam]

I was confused at why this would be necessary for the idata change. I reverted it (on top of c64ac9a) and tests still pass.

Looking more closely at the existing upstream code, eta_location is the result of std::find on the eta labels, so 0-based. By not considering j = 0, it mishandles an eta column if it's the first column. So this is relevant for data too and is an independent fix for a bug from gh-1037. Okay.

Fwiw here's a regression test that fails without the > to >= equal change:

test_that("etasrc works with ETA in first column", {
  mod <- param(mod, mode = 0)

  data <- expand.ev(amt = 100, ID = seq(4), cmt = 1)
  data <- mutate(data, ETA1 = rev(ID) / 10)
  data <- expand_observations(data, times = seq(5))
  data <- mutate(data, cmt = 0)

  set.seed(9812)
  expect_identical(
    mrgsim(mod, data, etasrc = "data"),
    mrgsim(mod, select(data, "ETA1", everything()), etasrc = "data")
  )
})

[kylebaron]

Thanks; yes, I discovered this when creating the tests for this PR. I should have created another PR or at least written it up in the message. I'm going to revert this change and make it clean in a new PR in case this comes up in the future.

[FelicienLL]

Thanks Kyle for providing this so fast. It should make things much easier and flexible. The code looks good to me.