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

what to do with evppi() when eff & cost have different lengths to input parameters inputs? #53

Closed
n8thangreen opened this issue Nov 2, 2023 · 6 comments

Comments

@n8thangreen
Copy link
Contributor

While writing the (regression) unit tests to replace the current internal evppi() function with the {voi} package version, an error occured. For the smoking example, the data I have has different number of rows for the cost and effectiveness matrices and the parameter posterior sample size. They should correspond. BCEA::evppi() actually doesn't throw an error so I don't know how it handles this or if its done correctly.

  • Am I understanding this correctly? What should we do about it?
  • Could recalc c,e using the posterior samples?
  • Could use the same fix thats already in BCEA::evppi() but not sure if this assumes row match e.g. first 500 of 1000?

This is the voi::evppi() error:

Error in check_outputs_matrix(outputs$c, inputs, "outputs$c") : 
  Number of rows of `outputs$c` (500) should equal the number of rows of `inputs` (1000)

This is the test code:

  data(Smoking, package = "BCEA")
  treats <-
    c("No intervention", "Self-help",
      "Individual counselling", "Group counselling")
  
  bcea_smoke <- bcea(eff, cost, ref = 4, interventions = treats, Kmax = 500)
  inp <- createInputs(smoking_output)
  EVPPI_smoke <- BCEA::evppi(bcea_smoke, param_idx = c(2,3), inp$mat, h.value = 5e-7)

  # error
  voiEVPPI <- voi::evppi(bcea_smoke[c("e","c","k")], pars = c("d.3.", "d.4."), inputs = inp$mat, h.value = 5e-7)
@n8thangreen
Copy link
Contributor Author

@giabaio any ideas?..

@giabaio
Copy link
Owner

giabaio commented Nov 2, 2023

I struggle to replicate this... Are you on the 2.4.4 version of BCEA? Is the object smoking_output available by default?

@n8thangreen
Copy link
Contributor Author

n8thangreen commented Nov 3, 2023

Hi,
You're right that I had taken out the jags output when sending to CRAN previously. I think I thought it was an issue but probably wasnt the actual problem.

I'd put it back in to the project recently to do the evppi testing, see:
n8thangreen@38ff5ff

I think this is the code taken wholesale from the BCEA book.

Smoking.Rmd is in the refactor-remove-evppi branch or I can send it to you separately?

@giabaio
Copy link
Owner

giabaio commented Nov 17, 2023

One more on this, before we can perhaps close this: I've modified the smoking_output object under data/Smoking.RData to select only the relevant matrix of simulations. This actually makes sense, because to use this object for the createInputs() and then evppi() example, we don't need it to be an rjags object --- it can be (as it can be a bugs or stan object, but a simple matrix will just do...). With this change (and by updating the GitHub repo for plotrix --- BTW: I think it's not orphaned any more, so perhaps we don't even need to add that repo in the Remotes field of our DESCRIPTION file) it now passes all the checks, except for AppVeyor. @n8thangreen given that we have included the check under Windows with the normal flow, do we need to keep AppVeyor? Or could we just remove that too?...

@n8thangreen
Copy link
Contributor Author

Thanks for fixing this. After syncing the my fork its passing all the CHECKS in the GH Action. I'll remove the AppVeyor thing; I think thats just a legacy thing anyway

@giabaio
Copy link
Owner

giabaio commented Nov 21, 2023 via email

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

No branches or pull requests

2 participants