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

Incorrect convolve mode in hospitaladmissions.py #385

Closed
damonbayer opened this issue Aug 15, 2024 · 9 comments · Fixed by #398
Closed

Incorrect convolve mode in hospitaladmissions.py #385

damonbayer opened this issue Aug 15, 2024 · 9 comments · Fixed by #398
Assignees
Labels
clean up Good code that could be better

Comments

@damonbayer
Copy link
Collaborator

damonbayer commented Aug 15, 2024

We should use mode="valid" to ensure the output reflects the entire infection_to_admission_interval being used.

https://github.com/CDCgov/multisignal-epi-inference/blob/main/model/src/pyrenew/latent/hospitaladmissions.py#L189-L193

I believe these lines should be replaced with

        latent_hospital_admissions = jnp.convolve(
            latent_hospital_admissions_raw,
            infection_to_admission_interval.value,
            mode="valid",
        )
@damonbayer damonbayer added the bug Something isn't working label Aug 15, 2024
@dylanhmorris
Copy link
Collaborator

I think the previous version was to allow the kind of behavior we've now decided to forbid. But I agree with forbidding it, at least by default.

@damonbayer
Copy link
Collaborator Author

The issue is actually more with the post-convolve indexing, but this change makes it easier to avoid that error.

@damonbayer damonbayer added this to the Q Sprint milestone Aug 19, 2024
@sbidari
Copy link
Collaborator

sbidari commented Aug 19, 2024

The issue is actually more with the post-convolve indexing, but this change makes it easier to avoid that error.

Can you point me to the issue/error that is being referenced here for my knowledge? @damonbayer @dylanhmorris

@damonbayer
Copy link
Collaborator Author

With mode="full" you will get results that implicitly pad the arrays with with 0's. When using mode="valid", this does not happen.

With mode="full", you then have to slice the head and the tail of the resulting array to get the desired output. With mode="valid" you only need the tail.

@sbidari
Copy link
Collaborator

sbidari commented Aug 20, 2024

With mode="full", you then have to slice the head and the tail of the resulting array to get the desired output. With mode="valid" you only need the tail.

The current implementation only includes slicing one end of the array resulting from convolution.

Using mode = "valid" results in latent_hospital_admissions array smaller than desired shape i.e latent_hospital_admissions_raw.shape[0]. Did you want to say mode = "same"?

@damonbayer
Copy link
Collaborator Author

damonbayer commented Aug 20, 2024

Using mode = "valid" results in latent_hospital_admissions array smaller than desired shape i.e latent_hospital_admissions_raw.shape[0]. Did you want to say mode = "same"?

No, the "valid" mode is correct. The desired shape in the end should be the length of the observed admissions. We can do that here or later on in the "model" code.

@sbidari
Copy link
Collaborator

sbidari commented Aug 20, 2024

The desired shape in the end should be the length of the observed admissions. We can do that here or later on in the "model" code.

Right! We want the final latent_hospital_admission same length as observed_hosp_admissions. But convolve mode = "valid" yields latent_hospital_admissions array with length smaller than observed_hosp_admissions.

@damonbayer
Copy link
Collaborator Author

But convolve mode = "valid" yields latent_hospital_admissions array with length smaller than observed_hosp_admissions.

What are the lengths of the relevant quantities?

@sbidari
Copy link
Collaborator

sbidari commented Aug 20, 2024

But convolve mode = "valid" yields latent_hospital_admissions array with length smaller than observed_hosp_admissions.

What are the lengths of the relevant quantities?

      latent_hospital_admissions = jnp.convolve(
          latent_hospital_admissions_raw,
          infection_to_admission_interval.value,
          mode="full",
      )[:latent_hospital_admissions_raw.shape[0]]
latent_hospital_admissions.shape 
# (105, )
      latent_hospital_admissions = jnp.convolve(
          latent_hospital_admissions_raw,
          infection_to_admission_interval.value,
          mode="valid",
      )
latent_hospital_admissions.shape 
# (51, )

observed_hosp_admissions.shape
# (90, )

@sbidari sbidari added clean up Good code that could be better and removed bug Something isn't working labels Aug 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clean up Good code that could be better
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants