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

Testing for key functions #54

Closed kaitejohnson closed 2 months ago

kaitejohnson commented 2 months ago

Goal is just to have some amount of unit tests of the functions in place. In this case, because I am planning on making some significant revisions in #49 , this PR is focused on testing the expected behavior of the pre-processing, the input data validation, and then the immediate validation/tests of the inputs being passed to stan. It is not intending to be comprehensive. I have added to the description in #49 to specify that that PR requires that all those functions be tested.

This will close #27, but also some scope creep/clean up will close #48, #17, #51, and #21.

kaitejohnson commented 2 months ago

@dylanhmorris Think I addressed all of your questions here, ready for review

kaitejohnson commented 2 months ago

Ok I am a bit stuck on this particular CI issue, seems to have randomly cropped up. Otherwise ready for your continued review @dylanhmorris

kaitejohnson commented 2 months ago

Looks great, @kaitejohnson! A few minor comments. Since you are adding testing, I wonder: Do you have the coverage information? That should be handy now.

Yeah, let me look into adding that into CI?

kaitejohnson commented 2 months ago

Ok @dylanhmorris I think I covered all of your suggestions/additional tests. I think any other tests should be new issues, this was mostly just to have some amount of testing infrastructure, not to be fully comprehensive and I am sure we will find bugs/more things to write tests for as we go.

@gvegayon I made a new issue for adding test coverage https://github.com/CDCgov/ww-inference-model/issues/63

kaitejohnson commented 2 months ago

LGTM. Feel free to merge. Did you want to move the forecast_date out of the model_spec in this PR or keep it for a separate one, @kaitejohnson.

I think it would be good to do in this PR! Sorry I must have missed a comment, do you think it makes sense to just make it a separate argument?

dylanhmorris commented 2 months ago

I think it would be good to do in this PR! Sorry I must have missed a comment, do you think it makes sense to just make it a separate argument?

Yes I do.