iSoron / uhabits

Loop Habit Tracker, a mobile app for creating and maintaining long-term positive habits
GNU General Public License v3.0
7.8k stars 933 forks source link

Fix SKIP behavior in YES_AUTO and score #1916

Open KristianTashkov opened 9 months ago

KristianTashkov commented 9 months ago

I've had some SKIPs over the Christmas holidays and noticed the score was more punishing than expected. I have a habit setup to be done three times every week. I expect that if I have skipped some days, they will be treated as non-existent days, and occurrences around them will connect. This is not what happens, though: current As you can see doing two occurrences and skipping the end of the week, doesn't count next week's occurence as the third in the chain, because it's too far away in the expected range. By changing the logic to treat skip_days as not existent days you can keep the habit going without being punished for having half-weeks + skips: fixed

This affects both YES_AUTO and the actual score calculation. @iSoron if you agree this should be fixed, I can update/write tests to cover it.

This behaviour has been mentioned before here: https://github.com/iSoron/uhabits/discussions/1686

wobbba commented 9 months ago

I'mma randomly drop in to add my two cents:

Though I initially thought that the chain should connect as you described, I have since adapted my use of skip days as a half-check (in my head anyway). E.g. I meditated but only 5min instead of 15. Also like this, skip days seem OP imo, since you can create really long chains without actually doing anything. But from the name, it makes more sense to treat them as nonexistent. Maybe to be consistent we would then also need to exclude them from the chain length? So the chain in your second example would only be of length 22 instead of 28 (relevant for longest chain view).

Is this related to the issue where SKIP-ing a YES_AUTO day sometimes makes the score go down?

KristianTashkov commented 9 months ago

Maybe to be consistent we would then also need to exclude them from the chain length? So the chain in your second example would only be of length 22 instead of 28 (relevant for longest chain view).

I've suggested this before and @iSoron didn't like it since "chain length" is the length of the time period without a lapse, not how many times you did it persay.

Is this related to the issue where SKIP-ing a YES_AUTO day sometimes makes the score go down?

Yes! It fixes that problem as well :) EDIT: Actually I'm not sure I have seen this one. The current code keeps the score on SKIP days the same as the previous one. But this PR fixes where score starts to go down after SKIP days even if you have enough occurrences

wobbba commented 9 months ago

It was just something I noticed sometimes but here are steps to reproduce:

This might be related to #1695 but that one seems closer to what you describe (it also mentions the issue you mentioned). In my case the skip day is within the implicitely checked range yet there is a drop in score. But possibly related. Do you think I should open another issue for this specifically?

KristianTashkov commented 9 months ago

Yes, that's a slightly different problem than https://github.com/iSoron/uhabits/issues/1695.

SKIP days keep the score the same as the previous day, but while a habit is still building, its score goes up on YES_AUTO days (while it converges to the occurrence counts you are doing). So, while adding a SKIP on those days doesn't decrease the score, it can reduce the rate of increase, leading to a lower score at the end.

An easy "fix" for this would be to make SKIP days take the higher of the "normally calculated" score or the previous score, but that goes against the "SKIP days are non-existent days" philosophy, which we seem to have. It would lead to score increasing indefinitely if you keep skipping and the week before the SKIPS you were doing well, which I don't think is something we want to have.

wobbba commented 9 months ago

That makes sense! I agree, if we go for "skip days are nonexistent" then the behaviour I described is actually intended.

iSoron commented 8 months ago

Thanks for the PR, @KristianTashkov. I agree that we should treat SKIPs as non-existing days. This would be consistent with keeping the score unchanged when you skip a day. Could you please add some tests to verify the desired behavior? I think the example you have in this PR would be a good test.