OasisLMF / ktools

In-memory simulation kernel for loss modelling.
BSD 3-Clause "New" or "Revised" License
28 stars 19 forks source link

Potential bug in AAL calculation with -c flag turned on #321

Closed jchhabra1C closed 2 years ago

jchhabra1C commented 2 years ago

As sidx gets greater than samplesize_, the inner loop will not break and erroneous values are read into mean variable resulting in incorrect calculation of AAL based on samples.

https://github.com/OasisLMF/ktools/blob/e77fc2dd436a7b49841c0506b26eb374a4ecd836/src/aalcalc/aalcalc.cpp#L362

hchagani-oasislmf commented 2 years ago

Please correct me if I have misunderstood. While it is true that the inner loop will not break if sidx exceeds samplesize_, I cannot see any circumstances under which sidx would exceed samplesize_. The initial value of sidx in the aforementioned loop is the key of vec_sample_aal_. These keys are set in void aalcalc::getsamplesize():

https://github.com/OasisLMF/ktools/blob/e77fc2dd436a7b49841c0506b26eb374a4ecd836/src/aalcalc/aalcalc.cpp#L155-L170

With the exception of the final key samplesize_, the other keys (where x is the key) are assigned in a loop: (x + (x - 1)) <= samplesize_ or in other words 2x - 1 <= samplesize_.

Therefore, if samplesize_ = 6, the keys are 1, 2, 6. There is no key of 4 because this would violate the above condition. Therefore, the loop breaks when sidx reaches (iter->first << 1) = 2 << 1 = 4.

If samplesize_ = 7, the keys are 1, 2, 4, 7. The loop now breaks when sidx reaches (iter->first << 1) = 4 << 1 = 8.

Putting it another way, it is the way sample sizes are assigned in aalcalc::getsamplesize() which means that the loop will break should sidx exceed samplesize_.

hchagani-oasislmf commented 2 years ago

@jchhabra1C, would you agree with the above?

jchhabra1C commented 2 years ago

Thanks for getting back. I was having this issue because I pulled the code before June 28 and this update should fix it https://github.com/OasisLMF/ktools/commit/5893b68a96af2f180c9143d1c01e03091bdff211#diff-17f95ddca6bfad4d8dacc1cec2f838f2b51b24331897ad4c56b617d7975e1ff3L169.