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

Update risk.py #67

Merged
merged 3 commits into from
Nov 11, 2024
Merged

Update risk.py #67

merged 3 commits into from
Nov 11, 2024

Conversation

drozzy
Copy link
Contributor

@drozzy drozzy commented Oct 2, 2023

Fixing #63

Copy link

@jinyu-loopmind jinyu-loopmind left a comment

Choose a reason for hiding this comment

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

In general, this is a big change on how to compute risk index. It might have big impact on people who are already used to the old scales, especially the risk index is no longer the sum of LBGI and HBGI. Risk index was computed as the sum of LBGI and HBGI for a long time in the original UVa/Padova Simulator.

To minimize the impact, I am thinking about introducing your new risk index computation as an alternative instead of completely replacing them. Maybe a more gentle way to do this is to issue deprecation warning, and advice people to use the new risk index instead. Let me know what you think on this. Thanks!

@drozzy
Copy link
Contributor Author

drozzy commented Oct 3, 2023

Do you know where in UVa/Padova Simulator they define it? I would be curios to see their implementation.

Regarding current use, I can't imagine people have used it because it produces wrong output at extreme values.

For example, here is the current risk if I let the glucose fall indefinitely:
original_extreme_values

As you can see the function just becomes undefined at some point and gets set to zero.

Here is the proposed risk function for the same trajectory:
proposed_extreme_values

Regarding compatibility, the new risk behaves the same for values in the acceptable range.
Here is the current risk:
original_risk

and here is proposed risk:
proposed_risk

For large glucose values, the current risk is not a big problem as it does increase in the desired direction, however it is not bounded and goes to infinity:
large_gluc_original_risk

the proposed risk caps this at 100:
large_gluc_proposed_risk

I welcome more testing and feedback of my proposal, but believe it is especially important for automated optimizers, where such a drop may lead to incorrect learned behavior.

@jxx123
Copy link
Owner

jxx123 commented Oct 22, 2023

Do you know where in UVa/Padova Simulator they define it? I would be curios to see their implementation.

Regarding current use, I can't imagine people have used it because it produces wrong output at extreme values.

For example, here is the current risk if I let the glucose fall indefinitely: original_extreme_values

As you can see the function just becomes undefined at some point and gets set to zero.

Here is the proposed risk function for the same trajectory: proposed_extreme_values

Regarding compatibility, the new risk behaves the same for values in the acceptable range. Here is the current risk: original_risk

and here is proposed risk: proposed_risk

For large glucose values, the current risk is not a big problem as it does increase in the desired direction, however it is not bounded and goes to infinity: large_gluc_original_risk

the proposed risk caps this at 100: large_gluc_proposed_risk

I welcome more testing and feedback of my proposal, but believe it is especially important for automated optimizers, where such a drop may lead to incorrect learned behavior.

Wow, thank you so much for the detailed and compelling analysis. Yes, the current risk computation is definitely problematic.

FYI, here is how the original UVa/Padova Simulator implements the risk index (they sum up LBGI and HBGI), where i means i-th patient.
Screenshot by Dropbox Capture

I think I will approve your change anyway since the existing one is definitely wrong. Thank you so much!

@drozzy
Copy link
Contributor Author

drozzy commented Oct 23, 2023

Out of curiosity, where did you find this code? Is it accessible somewhere?

@jxx123
Copy link
Owner

jxx123 commented Oct 25, 2023

Out of curiosity, where did you find this code? Is it accessible somewhere?

I had it as in the 2008 UVa/Padova Simulator Academic version. You might be able to request a version from epsilon group https://tegvirginia.com/software/t1dms-2014/, but I heard stories that there was no reply from them.

@jxx123 jxx123 merged commit d800ae1 into jxx123:master Nov 11, 2024
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.

3 participants