ukhsa-collaboration / covid-19-app-android-ag-public

COVID19 Android app
Other
142 stars 31 forks source link

Subtract mu (-0.3) from number of days as stated in equation 3 #6

Closed t-chappell closed 3 years ago

t-chappell commented 4 years ago

https://github.com/nhsx/covid-19-app-android-ag-public/blob/01f790a0ebefe20ba6ff7925e56dfd88911741bf/app/src/main/java/uk/nhs/nhsx/covid19/android/app/exposure/encounter/InfectiousnessFactorCalculator.kt#L12-L17

Changes to this function needed for results to be inline with the equation.

1) Make daysFromOnset a decimal/fractional data type for better accuracy of the duration part and use the correct start and end timestamps. 2) Add 0.3 to daysFromOnset as stated in the equation,

val step1 = (daysFromOnset + 0.3) / sigma

arcayn commented 4 years ago

The distribution mean mu0 = -0.3, and the mean is being subtracted from the numerator. Therefore, if this correction is to be made, I believe it should be to add 0.3, rather than to subtract it.

t-chappell commented 4 years ago

Not sure I follow, could be I'm missing something but the equation expressed in one line is:

exp( -0.5 * ( ( days::decimal - 0.3 ) / 2.75 ) ^ 2 )

What is currently in use:

exp( -0.5 * ( days::integer / 2.75 ) ^ 2 )

Below is equation 3 from the link in the comment above this function:

image

t-chappell commented 4 years ago

ah yes, - - 0.3, updated issue

mezpahlan commented 3 years ago

Apologies now I may not be understanding correctly..... but I think what we want is a mix of the original and the updated title :stuck_out_tongue_closed_eyes: . I realise there's a double negative thing going on but if you're looking at the equation and expecting the code to match I'd expect to see a minus in code not an addition.

So I think we should add mu as a constant in its negative form.

companion object {
  private const val sigma = 2.75
  private const val mu = -0.3 // Note the negative value here.
}

And then update the calculation like so:

// Calculated using equation 3 from the paper at https://arxiv.org/pdf/2005.11057.pdf
fun infectiousnessFactor(daysFromOnset: Int): Double {
  val step1 = (daysFromOnset - mu) / sigma
  val step2 = step1.pow(2)
  val step3 = -0.5 * step2
  return exp(step3)
}

I'm pretty sure the underlying language will understand the double negative and at least this way comparing equation 3 and the code is slightly more straight forward.

Apologies if this is off on the wrong direction. Either way, looks like IOS is using the same calculation.

t-chappell commented 3 years ago

Ah yeah you're right lol, no worries. Will update again.

Yeah the new constant should be added, but also the "daysFromOnset: Int" should be a decimal/float data type or casted before hand to work with decimal numbers.

Have had a look through other parts and seems everything is in whole number of days with a max of 7, seems odd.

arcayn commented 3 years ago

@mezpahlan introducing another constant is almost certainly the sane way to go about this, I was more just concerned with the fact that a constant wasn't being introduced and the calculation was being made less accurate as a result

t-chappell commented 3 years ago

any updates?

nhs-covid19 commented 3 years ago

Thanks for your interest in the NHS Covid-19 project. I have asked one of the GAEN API specialists on the team to answer your query.

They say: We use the transmissionRiskLevel field on the key to represent daysFromOnset, however GAEN API limits us to only being able to use the values 0-7 which means we can't use fractional or negative values. Because of not being able to send negative values, we send the absolute value of the difference in days. This abs(t) causes the curve to have a different shape and we decided that removing the mu parameter gave us a curve which was a closer approximation to the ideal The blue curve is the ideal curve, in red is if we included mu=-0.3 mu is -0 3

Again blue curve is ideal, red is the actual equation we use equation we use