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

Overview add Sensitivity OKDialog #3580

Open
wants to merge 8 commits into
base: dev
Choose a base branch
from

Conversation

Philoul
Copy link
Contributor

@Philoul Philoul commented Dec 4, 2024

Include for Autosens area the same kind of OKDialog that we have for Basal or IOB
Il allow a shorter string with DynISF and AutoISF below autosens (ISF for Carbs is hidden), And OKDialog improve the understanding with an explicit string.

I also included the possibility to include more information on Variable ISF calculation (don't know for DynISF if it could be usefull to see for ex the different TDD included for ISF calculation or BG contribution, but for AutoISF I think we will be able to include usefull information)

Currently String from APS plugin are not included (I will try to propose a "User Friendly String" for AutoISF, and if possible not a "Dev Friendly String" 😉 )

Screenshot_20241204-185447_AAPS.jpg

@olorinmaia
Copy link
Contributor

Thumbs up from me.
For default I think if autosens is enabled it should show in overview. If dynISFsens is enabled autosens should be moved to this pop-up to avoid confusion and lessen the lines in overview.

@MilosKozak
Copy link
Contributor

I'm not sure about this

@Philoul
Copy link
Contributor Author

Philoul commented Dec 6, 2024

I'm not at home this week-end, will check again everything on sunday afternoon 😉.

@Philoul
Copy link
Contributor Author

Philoul commented Dec 9, 2024

Built with latest commit (without dedicated UiThread), and works fine on my side 😉

@olorinmaia
Copy link
Contributor

olorinmaia commented Dec 11, 2024

This is how overview screen looks now if DynISF and DynTDDSensitivty is enabled. I think it's time to move some information away from the overview screen and into this Sensitivity OKDialog, it will make the overview screen less stretched and we have the possibility to explain more what each number is for.
image

My suggestion is to always show only one line, and move other information to the OkDialog:

If Dynamic Sensitivity = Enabled

In Overview:
Always show Profile ISF -> DynISF. Example: 6.0 -> 5.5

OKDialog:
Dynamic ISF for SMB algorithm: 5.5
Dynamic ISF for bolus calc and carbs absorption: 5.0
TDD based Sensitivity ratio: 97% (Enabled/Disabled)
Autosens: 100% (Disabled)
Estimated TDD: X

If Dynamic Sensitivity = Disabled

In Overview: (Only show Autosens %)
x%

OKDialog:
Nothing I can think of. (Disable OKDialog)

@olorinmaia
Copy link
Contributor

olorinmaia commented Dec 12, 2024

How it looks with mg/dl
Screenshot_20241212_132032_AAPSClient2

@Philoul
Copy link
Contributor Author

Philoul commented Dec 12, 2024

That's why I think average ISF should be moved to OKDialog...

  • this value is not understood alone (without explicit string)
  • variation is very slow
  • it increases a lot the width of the view

We can also probably adjust ISF rounding value with one decimal for mmol unit and no decimal for mgdl and reduce overall width.

@olorinmaia
Copy link
Contributor

olorinmaia commented Dec 12, 2024

Agreed, "ISF for carbs absorption" isn't a precise name though as it is also used in Bolus calculator BG/Trend calculation.

@MilosKozak @Philoul @vanelsberg @jbr7rr @ga-zelle
What are your thoughts about leaving only one line in Overview and moving everything else to the OKDialog? It's only when Dynamic Sensitivity is enabled that the need for all this information is handy. Why not gather it all in one Dialog box?

For me the DynISF used for SMB algo is most useful to see in Overview without having to click on sensitivity icon. ISF for bolus calc is also visible when opening bolus calc. Gathering all relevant sensitivity numbers in this box provides a better user experience imo, as we can explain them all abit more.

@Philoul
Copy link
Contributor Author

Philoul commented Dec 13, 2024

I like "only one line" with main information visible (AutoSens with OpenAPS SMB, no need to provide OKDialog), Profile and variable ISF Values for DynISF and AutoISF (then Additional info like Avg ISF, Autosens value, ISF detailed info...), could be different with DynISF and AutoISF...

"ISF for Carbs absorption" was a proposal (string short enough to have label and value in one ĺine).
My initial idea was to have an explicit label for avg ISF (not understandable "alone"), but also provide more detailed info on ISF calculation mainly for AutoISF and potentially DynISF too (everything is prepared to be able to include a custom string built within APS algo...)

@ga-zelle
Copy link
Contributor

ga-zelle commented Dec 14, 2024

Yes, one line only. Its content depends on the current plugin or mode. My vote is mostly percentage of final ISF compared to profile value with the exeption of active DynISF where it is shown as absolute value without showing the profile value in overview.

@vanelsberg
Copy link
Contributor

only one line

Have been on OpenSMB (which is also showing this) and found it quite informative as it shows my deviations on ISF from profile and actual ISF calculated? So for me, I'm quite happy with the view as is currently... 🤔

@robertrub
Copy link

robertrub commented Dec 14, 2024

Screenshot_20241214_115219_AAPS

I find that this corner is overcrowded. Even the last ) can't be seen and it pushes all the other numbers (COB IOB) to the left.

Either we can show one number and tap to have a window with more info (as in IOB) or we have to move around the numbers or/and make them smaller.

Screenshot_20241214_115603_AAPS

Move the AS next to the icon and delete the average in (). It can be seen with a tap in a new window as it is not very useful to have it all the time.

The above screenshots were with AutoISF.

Also, if I'm on OpenSMB with DynISF turned OFF, I don't need to see what my ISF would be if I had DynISF tuned ON... I'd like to get that info after a tap to get detailed info.

@robertrub
Copy link

And, the actual situation is not great with the widget either...

Screenshot_20241214_121112_One UI Home.jpg

@vanelsberg
Copy link
Contributor

And, the actual situation is not great with the widget either...

For me it is showing just fine. Try resizing the widget?
Or else, remove and readd. Widget could be on older cached version?

@vanelsberg
Copy link
Contributor

vanelsberg commented Dec 15, 2024

I find that this corner is overcrowded.

For me, it has value in a sense that I can see what algo is doing and what impact sensitivity has (or would have)?

A -> B (C)
A := ISF value used for algo calculation after adapting for sensitivity
B: = Change according to AutoISF
C: = Original ISF value in profile

(Note that currently Calculator is using C)
Maybe @ga-zelle or @Philoul can add to that?

@robertrub
Copy link

Or else, remove and readd. Widget could be on older cached version?

Nope, just deleted the widget and set it up again with the sale result

@robertrub
Copy link

robertrub commented Dec 15, 2024

A -> B (C)
A := ISF value used for algo calculation after adapting for sensitivity
B: = Change according to AutoISF
C: = Original ISF value in profile

What I need to see all the time is

modified ISF (profile ISF)

A tap on those numbers can give me all the details one can wish for.

Another positif side is that DynISF and AutoISF can have the same screen output.

On the main UI, above the ISF numbers, we can have the AutoSens % and a small icon to show if it is ON or OFF. The on/off information can be given by text color too.

@Philoul
Copy link
Contributor Author

Philoul commented Dec 15, 2024

@vanelsberg

A -> B (C)
A := ISF value used for algo calculation > after adapting for sensitivity
B: = Change according to AutoISF
C: = Original ISF value in profile

It's the proof it cannot remain "As is"

You are a skilled user, knows perfectly well AAPS, but your explains are wrong...
A: = Original ISF value in profile
B: = Change according to AutoISF or DynISF
C: = 24h average ISF value used for Carbs absorption or Calculator.

@vanelsberg
Copy link
Contributor

your explains are wrong.

Ah yes, I see now :-|

It's the proof it cannot remain "As is"
Thinking about the above, I tend to agree on that. So yes.... ;-)

@vanelsberg
Copy link
Contributor

A tap on those numbers can give me all the details one can wish for.

Agree, this could be a valid alternative.
It's just that for some info I need to be able to see in a glance (vs need for taking action to get it)

@vanelsberg
Copy link
Contributor

Nope, just deleted the widget and set it up again with the sale result

That is strange.
For me gadget it is showing just fine on S22 Ultra, S9 Note (lineageOS) and Nokia 5.4

@robertrub
Copy link

For me gadget it is showing just fine on S22 Ultra, S9 Note (lineageOS) and Nokia 5.4

Screenshot_20241215_112153_One UI Home.jpg

Screenshot_20241215_112218_AAPS.jpg

@ga-zelle
Copy link
Contributor

Or else, remove and readd. Widget could be on older cached version?

Nope, just deleted the widget and set it up again with the sale result

I had a similar squeeze problem where a whole line was missing. Solved by reducing the general font size of the phone.

@robertrub
Copy link

robertrub commented Dec 16, 2024

Yes, if I make the font smaller (2nd position), it fits. But, I'm only using the third position out of 8 possible and I'm 62 y.o. 😉
Screenshot_20241216_082102_Settings.jpg

@Philoul
Copy link
Contributor Author

Philoul commented Dec 16, 2024

An idea this morning for single line short info in Overview:

Icon above AS value has a double Arrow (with or without X) according to AS disabled or enabled.
This icon can be a bit more dynamic:

  • no change when 100% (should be most of the time)
  • Up arrow only (with or without X) when percentage is above 100%
  • Down arrow only (with or without X) when percentage is below 100%

OpenAPS AMA or SMB:
AS: xx% (consistant with dynamic icon above)
or
Alg: xx% (consistant with dynamic icon above)
Note: I still have to analyze code to understand the difference between AS and Alg % and when these values are not the same 🤔

In OKDialog (TBC if we need it or not)

  • explicit label and percentage for AS
  • explicit label and percentage for Alg

DynISF or AutoISF:
One line with ISF value (and dynamic icon above for AS).

  • I think we can keep xxx->yyy (same as current master)

In OKDialog:

  • AS/Alg values (single or 2 lines with explicit label)
  • Average ISF (with explicit short label)
  • Then, according to selected algo (DynISF or AutoISF), a user friendly string to explain ISF calculation (to be discuss)

Widget is a side effect of the too long string and should be fixed if we reduce the size 😉

@ga-zelle
Copy link
Contributor

Regarding the Alg: %% (or middle row if present) I think this comes from TT changing sensitivity

@Philoul
Copy link
Contributor Author

Philoul commented Dec 17, 2024

PR updated:

  • 4 new icons for autosens above or below 100%, with autosens enabled or not
  • string added for Autosens short (AS: ), long (in OKDialog) and Alg: (OKDialog only)

AS always visible with openAPS SMB

With DynISF or AutoISF

  • Single ISF Line with Profile and variable
  • AS value visible if outside 20% of autosens min or max value (my latest proposal)

Screenshot_20241217-230701_AAPS.jpg

Example with AS 111% (with autosens range 70%-130%, AS is only hidden between 94%-106% so visible here)
Screenshot_20241217-230819_AAPS.jpg

Screenshot_20241218-005448_AAPS.jpg

@@ -430,6 +430,11 @@
<string name="formatPercent">%1$.0f%%</string>
<string name="basal">Basal</string>
<string name="basalpct">Basal %</string>
<string name="sensitivity">Sensitivity</string>
<string name="isf_for_carbs">ISF for carbs absorption: %1$.1f</string>
Copy link
Contributor

Choose a reason for hiding this comment

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

How could we improve this string without making it to long? This ISF value is also used in Bolus calculator BG / Trend. This is very relevant information that we should present some how.

ISF for bolus and COB calculation
ISF for bolus calculator and COB calculation
ISF for bolus calculator and carbs absorption
ISF for bolus calc and carbs absorption

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Difficulty is none of your proposal fit in one line only for label + value (and they are in english language)
We should also think of translation (even if not directly under control), that can be longer...
All these label re in string so can be adjusted later if everyone agree on a better proposal ;-)

Copy link
Contributor

Choose a reason for hiding this comment

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

I would go for this alternative text to cover that this ISF value is used in bolus calc:
"ISF for bolus calc and carbs absorption" or "ISF for bolus and COB calculation"
It's not much longer then
"ISF for carbs absorption"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's enough yo have string in 2 lines instead of 1, at least on my phone... (1080x2340 pixel), so I can't imagine on a low res screen...
Did you try on your side?

Copy link
Contributor

@olorinmaia olorinmaia Jan 6, 2025

Choose a reason for hiding this comment

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

I haven't tried. But for my part there is not a big deal if string goes into two lines as there is plenty of space.

OpenAPS log can cover half the screen some times.

@olorinmaia
Copy link
Contributor

olorinmaia commented Dec 18, 2024

Thumps up from me. I think we all should have some win in it regardless of opinions :) and a good start that can be built upon. My only suggestion is try to improve the string about ISF for bolus calc/cob calc, see comment above.

P.S. some merge conflicts, so hard to test it

…tivity

# Conflicts:
#	plugins/main/src/main/kotlin/app/aaps/plugins/main/general/overview/OverviewFragment.kt
@Philoul
Copy link
Contributor Author

Philoul commented Dec 18, 2024

P.S. some merge conflicts, so hard to test it

I saw that, it's fixed...

@Philoul
Copy link
Contributor Author

Philoul commented Dec 18, 2024

Widget updated (within widget I kept everything always visible, so AS: xxx%, Alg: xxx% and variable sens are visible whatever the precentage value)
The only update is Autosens Arrow that is dynamic and "AS:" or "Alg:" are managed by a String for translation ;-)

Feel free to test and give me comments...

@ga-zelle
Copy link
Contributor

As a method of rapid prototyping I once created a spreadsheet to show what the OKDialog box could look like under various conditions. That avoids the whole cycle of editing, building, testing and waiting for the relevant condition to occur.

Here is a version with a bit more detail on AutoISF which is still under discussion between @Philoul and me. The DynISF case is bare bones because I don't know enough about the various conditions and combinations.
grafik

@olorinmaia
Copy link
Contributor

olorinmaia commented Dec 19, 2024

Another thing to think about is AAPSClient. Edit: not entirely sure how this work without PR so will test abit more.

@Philoul
Copy link
Contributor Author

Philoul commented Dec 19, 2024

This first PR will be limited to AS, Algo and avg ISF only.
Code is ready to include additional info from APS algo, but I think it's another topic.
Concerning AAPSClient version, I tried to remain consistant with what was previously coded, but you are right, it should be tested.

@Philoul
Copy link
Contributor Author

Philoul commented Dec 19, 2024

@olorinmaia your screenshot with inconsistancies between AAPS AS value and AAPSClient AS value is not linked to this PR, so should be managed outside...

What is important here fot this PR is the consistancy in AAPSClient between what we had before (2 or 3 lines) and what we have now (one line most of the time + OKDialog) 😉

@olorinmaia
Copy link
Contributor

olorinmaia commented Dec 19, 2024

I realized I can't test this right now because I am missing TDD on my test rig. Are you able to or someone else?

Dynamic sensitivity is enabled and functioning:
In AAPSClient we should be able to see dynISF for smb algo, ISF bolus calc and carb absorption and AS %. (like in dev without this PR)

@olorinmaia
Copy link
Contributor

Functionality tested OK with AAPSCLient. AS%, DynISF and DYnISF for carb absorption sync ok. Also the new arrows.
Some screenshots:

AAPS AAPSClient
image image

@MilosKozak
Copy link
Contributor

I tested this PR and AS is only in dialog.
I'd prefer see AS instead of ISF change as this is value valid for 5 min and AS is a long term trend
image

@Philoul
Copy link
Contributor Author

Philoul commented Dec 29, 2024

With latest version AS is hidden when 100% (most of the time), but you have the double arrow above.

Then when AS is less than 20% to autosens min or max you only have the simple arrow above (up or down) and value is also hidden...
AS is always visible above this 20%
For ex if autosensMax is 130% then you only have arrow between 101% and 106% (if above 106% you have AS value)...

Screenshot_20241229-205709_AAPS.jpg

I put a 20% limit (arrow above gives a visual information if AS is a bit above or below 100%), but if you prefer I can reduce limit or always show AS if not equal to 100%.

With dynamic icon above, I think we can hide AS when equals 100% (should be most of the time)...

@MilosKozak
Copy link
Contributor

ok
how much do look at DynISF? I added it initially for testing only but I'm looking at it very rarely

@olorinmaia
Copy link
Contributor

https://www.facebook.com/share/p/15boWpDu2s/

One of many post recent days about all the information shown under the sensitivity icon.

What if we only show AS: X% in overview screen. And if using dynamic sensitivity we also show Alg: x% if enabled.

But, imo we should use the okdialog to explain all these numbers properly also. The text should be so long that users understand.

Autosens: X% (Should we inform when in dynisf mode that it has no effect?)
TDD-based sensitivity ratio: X% (only show if dyn sens and tdd based sens is enabled)
Profile ISF:
Dynamic ISF for SMB algorithm:
Average Dynamic ISF last 24h for bolus calc and carb absorption:
Estimated TDD:

@ga-zelle
Copy link
Contributor

ga-zelle commented Jan 4, 2025

In a first implementation it suffices to show 3 or 4 numbers appearing now in overview and reduce that to just one piece of info like final isf %.

We cannot explain "everything" in that dialog box anyway.

@olorinmaia
Copy link
Contributor

olorinmaia commented Jan 4, 2025

This is not about explaining everything, but in my opinion the minimum required, and it is not much text. TDD isn't high priority, but it would be nice there to have eventually. These numbers are pretty important, and it shouldn't be needed to ask on facebook/discord or search in wiki to understand what's going on.

Luckily I can translate it as I wish in Norwegian, but base-line and most used is English, so we should try to explain these numbers as good as possible in the OKDialog box.

@robertrub
Copy link

I'm not sure if showing sensitivity (autosens) percentage when the function is turned OFF is really necessary.

I turned it off as it really doesn't fit me (shows high percentage when I'm already lowish and definitely got more insulin than needed). So, seeing a wrong % showing is really not helpful.

@ga-zelle
Copy link
Contributor

ga-zelle commented Jan 4, 2025

@robertrub Now that you used AutoISF for a while which single info would you like to see on the overview screen?
And what detail should go into the dialog panel to help you?

@robertrub
Copy link

robertrub commented Jan 5, 2025

Screenshot_20250105_025659_AAPS.jpg

This is the only information I need to see all the time: Profile ISF changed to AutoISF value.
All the other stuff like ISF used to calculate carbs and (why not) the AutoSens % can be shown only after a tap on the above ISF numbers.

Instead of the 2 arrows for AutoSens, we can have an AutoISF icon too but that really is only aesthetic.

@Philoul
Copy link
Contributor Author

Philoul commented Jan 5, 2025

Hum, no added value to have an AutoISF (DynISF) Icon here...

I think the dynamic Arrow (not always "Double" but "Up" or "Down" according to Autosens value is more usefull).

Using AutoISF, it's the same for me, most important info are the 2 ISF values (profile -> AutoISF).
But I like having AS too when far from 100%...

Current PR version includes an "automatic threshold" hardcoded to 20% of Min/Max Autosens values, to show or not AS value (below this threshold you only have the up or down arrow, or double arrows if equals 100%).

  • we can include an additional parameters to have a customized threshold to show or not AS value (0 = always visible to xx% = allways visible if above min/max Autosens param) or adjust the limit to show more or less AS value 🤔.

Note that for widget, I kept almost everything (AS and ISF always visible).
=> I just hide average ISF and included the dynamic Up or Down Arrow above AS value.

<string name="isf_for_carbs">ISF for carbs absorption: %1$.1f</string>
<string name="autosens_long">Autosens Value: %.0f%%</string>
<string name="autosens_short">AS: %.0f%%</string>
<string name="algorithm_long">Autosens in Loop: %.0f%%</string>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion to improve it:
Autosens used in algorithm

To align with short name "Alg"

…tivity

# Conflicts:
#	plugins/main/src/main/kotlin/app/aaps/plugins/main/general/overview/OverviewFragment.kt
Copy link

sonarqubecloud bot commented Jan 6, 2025

@Philoul
Copy link
Contributor Author

Philoul commented Jan 6, 2025

Within latest update, String has been modified (see below before and after to compare) and AS is now always visible if not equal to 100%.
image

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.

6 participants