search

CDCgov / ww-inference-model

An in-development R package and a Bayesian hierarchical model jointly fitting multiple "local" wastewater data streams and "global" case count data to produce nowcasts and forecasts of both observations
https://cdcgov.github.io/ww-inference-model/
Apache License 2.0
16 stars 2 forks source link

Switch to placing prior on and inferring `i/n` at the first observed timepoint #85

Closed dylanhmorris closed 1 month ago

dylanhmorris commented 1 month ago

Instead of the first unobserved timepoint. This should increase interpretability.

Closes #84

dylanhmorris commented 1 month ago

Thank you so much for doing this @dylanhmorris! I think this is definitely more intuitive. See though my suggestion about continuing to use the i0 notation. Curious your thoughts on that.

I think as long as we're clear about what we mean by t=0 it's fine either way. I went with first_obs for backward compatibility and to avoid breaking anything accidentally. If we do want to change time indexing I think that's a separate PR.

Also we should make sure to change the model definition alongside this. I think it was just ambiguous previously. I can branch off of this and try changing it to reflect the new parameterization if that would be helpful?

Yes, I think that would be good.

Can you describe a bit what the major change is in the test of the ihr_transform and rt_assmebly -- is it just that we want to use the get_ind_m() function instead of the one passed into stan (that upstream called that function) so that we are more explicitly testing that function?

Yes, the idea there is that makes the tests of the two assembly functions less dependent on the particular matrix used in / computed for the toy data. Longer term could/should try multiple. See also #109

kaitejohnson commented 1 month ago

Thank you so much for doing this @dylanhmorris! I think this is definitely more intuitive. See though my suggestion about continuing to use the i0 notation. Curious your thoughts on that.

I think as long as we're clear about what we mean by t=0 it's fine either way. I went with first_obs for backward compatibility and to avoid breaking anything accidentally. If we do want to change time indexing I think that's a separate PR.

Call me crazy, but I think we can still specify this as i0 from the user define parameters and the variable names, when under the hood the time indexing starts at -uot. I see why you switch to first_obs though. I can do it in a separate PR, but I just find it more readable as i0

dylanhmorris commented 1 month ago

i0 from the user define parameters and the variable names, when under the hood the time indexing starts at -uot. I see why you switch to first_obs though. I can do it in a separate PR, but I just find it more readable as i0

Yes, absolutely, but it should be a separate PR imo.

dylanhmorris commented 1 month ago

Weirdly CI isn't telling me what failed..

It's the snapshot test (test_ww_model.R). I go back and forth on whether to improve it or abandon it, but either decision is beyond the scope of this PR imo.

kaitejohnson commented 1 month ago

Weirdly CI isn't telling me what failed..

It's the snapshot test (test_ww_model.R). I go back and forth on whether to improve it or abandon it, but either decision is beyond the scope of this PR imo.

I think the thing that might have broke it was using the generate_initial_values = TRUE... again not ideal to have that off but I think the draws should be deterministic without that if the stan seed is set

dylanhmorris commented 1 month ago

I think the thing that might have broke it was using the generate_initial_values = TRUE... again not ideal to have that off but I think the draws should be deterministic without that if the stan seed is set

That was actually one thing I changed. It now generates initial values but fixes a seed. I strongly suspect the issue was architectural differences between the machine generating the snapshot and the machine running in CI, since I was able to resolve it by generating the snapshot somewhere else. This is the same thing @cbernalz ran into a while back. Again, it suggests a need to rethink the test a bit.