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

Avoid divide by zero error when there are no non-index infections #52

Merged
merged 4 commits into from
Dec 24, 2024

Conversation

afmagee42
Copy link
Contributor

Probably overkill, but now we won't have problems even if there are no simulations at all.

@afmagee42 afmagee42 requested a review from swo December 20, 2024 17:23
@afmagee42 afmagee42 linked an issue Dec 20, 2024 that may be closed by this pull request
Copy link
Contributor

@swo swo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo should be fixed. Using numpy rather than frac I leave up to you

ringvax/app.py Outdated
@@ -268,19 +282,11 @@ def app():
)

st.write(
"The following table provides summaries of marginal probabilities regarding detection. Aside from the marginal probability of active detection, these are the observed probabilities that any individual is detected in this manner. The marginal probability of active detection excludes index cases, which are not eligible for active detection."
"The following table provides summaries of marginal probabilities regarding detection. Aside from the marginal probability of active detection, these are the observed probabilities that any individual is detected in this manner. The marginal probability of active detection excludes index cases, which are not eligible for active detection, including the index case."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"The marginal probability of active detection excludes index cases, which are not eligible for active detection, including the index case."

I think this is just a typo? Index case is mentioned twice, once excluded and once inclusively ineligible

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this also highlights that perhaps what we really want, with respect to some of the discussion in #33, is to have 5 summaries, or maybe 6:

  • Pr(index detected) (this is just the passive detection probability, accounting for the delay)
  • Pr(non-index detected)
  • Pr(non-index detected actively)
  • Pr(non-index detected pasively)
  • Pr(any case detected before it's infectious)
    • If 6 summaries, this is two: Pr(index case detected before it's infectious) and Pr(non-index case detected before it's infectious)

@@ -26,6 +26,17 @@
assert set(infection_schema.keys()) == Simulation.PROPERTIES


def frac(num, denom) -> float:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the behavior of numpy.divide, so we could use that?

It throws a user warning, but you can toggle that off for divide by zero warnings with https://numpy.org/doc/2.1/reference/generated/numpy.seterr.html#numpy.seterr

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's also funny to me how much I would prefer

if num == 0.0 and denom == 0.0:
  return nan
elif num > 0.0 and denom == 0.0:
  etc.

What/when/where I developed that preference?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we can locally turn off the warnings with @np.errstate (otherwise we'd risk other things, like logs of negative numbers not warning elsewhere) so I'm deprecating frac in favor of divide plus a decorator on summarize_detections()

@afmagee42 afmagee42 merged commit a408fb1 into main Dec 24, 2024
2 checks passed
@afmagee42 afmagee42 deleted the afm-49 branch December 24, 2024 01:17
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

Successfully merging this pull request may close these issues.

Summary of detections triggers division by zero
2 participants