Repository navigation
Insights: don't count never-answered journal days as "without" - #682
digitalerdude wants to merge 1 commit into
Conversation
…r#631) The Behaviour Effects with/without split (BehaviorInsights.effect / EffectRanker, Swift and Kotlin) partitioned every outcome-day into "with" or "without" based solely on whether a journal question was answered "yes" — any day it was never asked/answered fell into "without" alongside genuine "no" answers. For a rarely logged question this makes "without" wildly outnumber real "no" days (a user's screenshot showed with n=3 vs without n=305 for a question asked on a handful of days), producing the misleading correlation stats ryanbr#631 reports. Thread an `askedDays`/`asked` set alongside the existing "yes" set so a day only counts in with or without when it was actually answered; a never-asked day is now excluded from both groups. Applied to the shared Swift BehaviorInsights/ EffectRanker engine, its Kotlin EffectRanker twin, and the still-separate non-shared partition logic inside Android's InsightsScreen.kt (old Insights tab), across all six call sites that build the behaviour-days maps (both platforms' Insights tab, Insights Hub, and AI coach).
|
Bewaren: the screenshot I shared in #631 showed a 100% mismatch. I mean: I answered each question for over 300 days (asked days!) with yes or no (so BehaviorDays). So not only the empty states were mismatched, ALL answers were ignored. I hope this is clear. |
ryanbr
left a comment
ryanbr
left a comment
There was a problem hiding this comment.
The never-asked partition fix itself is solid — clean, byte-parity across Swift/Kotlin (the guard contains||contains / !contains && !contains pair is equivalent), the OR-gate never drops a "yes" day, asked[name] ?? days is a safe fallback, and askedDays is correctly not lag-shifted. Good tests both sides. As a standalone improvement (stop never-asked days inflating "without") it's worth having.
But I don't think it fixes #631 as reported, and I'd hold the Fixes #631 on that.
#631 is explicit: Android, "imported journal entries from whoop are all labeled as 'without', however many were actually yes-es", and "new [native] entries are [correct]". bartmuskala's comment here ("ALL answers were ignored") says the same.
That's an import-specific symptom — imported yes → without while native logging works. A source-agnostic partition change treats imported and native days identically, so it can't be why import breaks but native doesn't. The real cause is in the import path's yes→answeredYes population (Android WhoopCsvImporter.kt / the WHOOP export's answer spelling or day-mapping), not the with/without split. Notably this PR flags a yes/no-parse bug but declares it out of scope and calls it "Swift-only, Android is fine" — yet the reporter is on Android with native-ok/import-broken, so the partition fix can't be the full explanation.
Suggestion — two clean options:
- Re-scope to actually fix the Android import yes-parse /
answeredYespath (validated against bartmuskala's attached.zip), and the partition fix rides along; or - Land the partition fix on its own — retitled, worded Addresses not Fixes #631, referencing a new issue for never-asked inflation — and keep #631 open for the import root cause.
Either way, worth confirming against the reporter's export before closing #631, since they've already said this doesn't match their data. It's a draft and the code is good, so this is just a comment — no changes requested on the diff.
|
Good catch, and thanks for flagging it before it got merged on a wrong "Fixes" claim. Agreed the partition fix can't be the explanation here — bartmuskala's comment confirms it's not a never-asked-days problem at all: he answered every question directly for 300+ days and the import still zeroed all of them out, while native logging is fine. That's import-specific, and this fix treats imported/native days identically, so it doesn't touch the actual reported symptom. Going with option 2: retitling/rewording this PR as a standalone partition fix (worth having on its own, per your read of the diff) and no longer claiming to fix #631. Leaving #631 open for the real import root cause. Best lead so far, not yet confirmed: |
digitalerdude
changed the title
Both the Android and Swift CSV journal importers looked up the answer
column only as answered_yes_no / answer / answer_text. That matches
NOOP's own exporter (which writes "Answered yes/no"), but a real WHOOP
export names the column "Answered yes" -> answered_yes, which never
matched. Every imported journal answer silently read false ("Without"
in Insights), regardless of what the account actually answered -
confirmed against a reporter's real journal_entries.csv (300+ days,
every answer zeroed out, native logging correct).
Fix: add answered_yes as a (first) candidate key on both platforms.
Added a regression test on each side using the real header and
TRUE/FALSE casing from the reporter's export.
Does not fully close #631 by itself if the reporter's export also has
other quirks, but fixes the confirmed root cause. Doesn't touch #682
(the separate never-asked-vs-no partition fix already split out from
this issue).
…r#631) Both the Android and Swift CSV journal importers looked up the answer column only as answered_yes_no / answer / answer_text. That matches NOOP's own exporter (which writes "Answered yes/no"), but a real WHOOP export names the column "Answered yes" -> answered_yes, which never matched. Every imported journal answer silently read false ("Without" in Insights), regardless of what the account actually answered - confirmed against a reporter's real journal_entries.csv (300+ days, every answer zeroed out, native logging correct). Fix: add answered_yes as a (first) candidate key on both platforms. Added a regression test on each side using the real header and TRUE/FALSE casing from the reporter's export. Does not fully close ryanbr#631 by itself if the reporter's export also has other quirks, but fixes the confirmed root cause. Doesn't touch ryanbr#682 (the separate never-asked-vs-no partition fix already split out from this issue).
…r#631) Both the Android and Swift CSV journal importers looked up the answer column only as answered_yes_no / answer / answer_text. That matches NOOP's own exporter (which writes "Answered yes/no"), but a real WHOOP export names the column "Answered yes" -> answered_yes, which never matched. Every imported journal answer silently read false ("Without" in Insights), regardless of what the account actually answered - confirmed against a reporter's real journal_entries.csv (300+ days, every answer zeroed out, native logging correct). Fix: add answered_yes as a (first) candidate key on both platforms. Added a regression test on each side using the real header and TRUE/FALSE casing from the reporter's export. Does not fully close ryanbr#631 by itself if the reporter's export also has other quirks, but fixes the confirmed root cause. Doesn't touch ryanbr#682 (the separate never-asked-vs-no partition fix already split out from this issue).
Summary
Standalone fix, split off after review discussion on this PR — does not fully resolve #631 (see below), but addresses a real, separate bug in the same area.
The "Behaviour Effects" with/without split (
BehaviorInsights.effect/EffectRanker, both platforms) partitioned every outcome-day into "with" or "without"based solely on whether a journal question was answered "yes" — a day it was never
asked/answered fell into "without" right alongside a genuine "no" answer. For a rarely
logged question this makes "without" wildly outnumber real "no" days. A screenshot
attached to #631 shows exactly this:
with n=3vswithout n=305for a question askedon only a handful of days — the 305 are almost all "never answered", not "no".
This is not the #631 root cause. bartmuskala confirmed (in the PR comments) that
he answered every question directly for 300+ days and imported answers were still all
mislabeled, while native logging is fine — an import-specific symptom this
source-agnostic partition fix can't explain. #631 stays open for that; current lead is
a missing Dutch locale entry in the CSV header/value alias tables (
CsvParser.kt/CSVParsing.swift), pending confirmation from the reporter's actual export.The earlier #136 fix only corrected which day a journal entry lands on (onset evening
→ wake day); it never touched this with/without partition, so the underlying mislabeling
survived independently of this bug.
Fix
Thread an
askedDays/askedset alongside the existing "yes" set through the effectcomputation, so a day only counts in
withorwithoutwhen it was actuallyanswered (yes or no) — a never-asked day is now excluded from both groups, on both
platforms:
BehaviorInsights.effect/.rank(Packages/StrandAnalytics) andEffectRanker.bestLag/.rank.com.noop.analytics.EffectRanker.effect/.bestLag/.rank(thebyte-identical port), plus the separate, non-shared partition logic still living
inline in
InsightsScreen.kt(the older Insights tab never called the sharedEffectRanker).tab, the v5 Insights Hub, and the AI coach) now build an "asked" set in the same
loop, no extra DB query.
The partition change is additive/defensive:
behaviorDaysORaskedDaysmembershipgates a day into the comparison, so a "yes" day is never dropped even if a caller's
askedDaysbuild has a gap.Out of scope (separate, unrelated, and less-verified issue — flagging for a future
PR rather than bundling):
Strand/Data/WhoopImporter.swift:156does an exact-match.lowercased() == "true"when parsing the imported CSV's yes/no column, which wouldmis-parse any other real-world spelling (e.g. "Yes"/"1") of a positive answer. It's
Swift-only (Android's
parseYesNoalready accepts "yes"/"1"/"y") and unrelated tothis partition bug.
Test plan
swift testinPackages/StrandAnalytics— full suite (1113 tests) passes,including new/migrated
BehaviorInsightsTestsandEffectRankerTestscasescovering: a never-asked day excluded from both groups, an asked-"no" day still
counting as without, the defensive OR-gate, a realistic sparse-question shape
matching the Imported journal entries are not parsed correct #631 screenshot (asked on 5/300 days), and
askedDaysstayingun-shifted across the lag search.
./gradlew testFullDebugUnitTest— full Android unit suite (2866 tests)passes, including the mirrored
EffectRankerTestcases../gradlew compileFullDebugKotlin— Android app module compiles.xcodegen generate && xcodebuild -scheme Strand -destination 'platform=macOS' CODE_SIGNING_ALLOWED=NO build— macOS app target (which the changedInsightsView.swift/InsightsHubView.swift/AICoach.swiftlive under, notcovered by
swift-packages.yml) builds clean.fix is pure in-memory analytics over already-persisted journal data, no
migration/schema/backup involved.