feat: infer substance for unit-only dose entries#7
Conversation
Entries that give only an amount and unit with no named substance were either dropped as parse errors or mis-parsed with the unit itself as the substance. Now: - the grammar accepts unit-only doses (substance is optional) - load infers the substance from the most common substance recorded with that unit (and ROA, when the entry specifies one), assigning only when there is a clear winner and warning otherwise; the dominant ROA is filled in for inferred entries Also includes related parser fixes surfaced by this change: - treat 'hit'/'hits' as units (matching the dose-unit definitions) - accept approximate percentages in notes, e.g. "(~5%)" - recognize ratio notes like "(10:1)" before attempting to parse a dose, so the now-optional substance does not turn them into phantom subdoses
|
@greptileai review |
1 similar comment
|
@greptileai review |
Greptile SummaryThis PR makes
Confidence Score: 5/5Safe to merge; all changed code paths are guarded, tested, and integrated correctly into the load pipeline. The grammar change (optional substance), inference logic, and drop-on-failure behaviour are all correct and well-tested. The two-pass frequency-map approach avoids any feedback loop, the _clear_winner thresholds are conservative, the plural-normalisation allowlist correctly replaces the earlier blanket suffix-strip, and unresolvable entries are cleanly dropped rather than silently persisted. No new data-loss or mis-classification paths were found across the changed files. No files require special attention. Important Files Changed
Reviews (3): Last reviewed commit: "refactor: address review on implicit-sub..." | Re-trigger Greptile |
![greptile-apps[bot]](https://cdn.github.com/in/867647?s=60&v=4){kind=small}
| def _norm_unit(unit: str) -> str: | ||
| """Normalize a unit for substance inference, collapsing simple plurals | ||
| (e.g. unit/units) so singular and plural share an inference key.""" | ||
| unit = unit.lower() | ||
| if len(unit) > 1 and unit.isalpha() and unit.endswith("s"): | ||
| unit = unit[:-1] | ||
| return unit |
There was a problem hiding this comment.
_norm_unit strips a trailing s from any alphabetic string longer than one character. This works correctly for all current grammar units (puffs→puff, hits→hit, servings→serving), but units that legitimately end in s as their singular form (e.g., a hypothetical "bonus" or "IUs") would be incorrectly normalized, silently merging distinct inference buckets. Consider using an explicit allowlist of known plural units instead of the blanket suffix rule.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Good call — replaced the blanket trailing-'s' strip with an explicit plural→singular allowlist (_UNIT_PLURALS) in a9ac780, so units that legitimately end in 's' won't be mangled.
| for e in events: | ||
| if e.type != "dose" or e.substance: | ||
| continue | ||
| dose = e.data.get("dose", {}) | ||
| unit = dose.get("unit") | ||
| if not unit or unit == "unknown": | ||
| continue | ||
| unit = _norm_unit(unit) | ||
| roa = dose.get("roa") | ||
|
|
||
| winner = None | ||
| if roa and (unit, roa) in by_unit_roa: | ||
| winner = _clear_winner(by_unit_roa[(unit, roa)]) | ||
| if winner is None: | ||
| winner = _clear_winner(by_unit[unit]) | ||
| if winner is None: | ||
| unresolved[unit] += 1 | ||
| continue | ||
|
|
||
| e.data["substance"] = winner | ||
| e.data["implicit_substance"] = True | ||
| if not roa: | ||
| roa_winner = _clear_winner(roa_by_unit_sub[(unit, winner)]) | ||
| if roa_winner: | ||
| dose["roa"] = roa_winner | ||
| n_inferred += 1 | ||
|
|
There was a problem hiding this comment.
type="dose" with substance=None
Entries that fail inference remain in the pipeline as type="dose" events with substance=None. Whenever any caller accesses e.dose on such an entry, event.py's dose property fires assert self.substance → AssertionError → print(self.data) to stdout plus a log.warning — once per access, per unresolved entry. Consider either changing the event type for unresolved entries or guarding the e.dose property against substance=None without printing.
There was a problem hiding this comment.
Fixed in a9ac780: unit-only entries that can't be resolved are now dropped (with one aggregate warning) instead of lingering as substance=None "dose" events, so Event.dose never hits the assert/print path. Verified on real data: 0 substance=None dose events remain after load.
- _norm_unit: use an explicit plural->singular allowlist instead of a blanket trailing-'s' strip, which could silently merge unrelated unit buckets (e.g. a unit whose singular legitimately ends in 's') - _infer_implicit_substances: drop entries that have no substance and for which none can be inferred, instead of leaving them as substance=None "dose" events (which made Event.dose emit a warning + stdout print on every access). Returns the filtered list; logs one aggregate warning.
|
@greptileai review |
1 participant
Summary
Entries that give only an amount and a unit with no named substance were
being mishandled by the parser: depending on whether the unit was already in
the grammar, they were either dropped as parse errors or mis-parsed with
the unit itself as the substance. On real logs this silently lost or
mislabeled a large fraction of such entries.
This PR makes the substance optional and infers it from context:
load._infer_implicit_substances): for a unit-only entry, thesubstance is inferred from the most common substance recorded with that
unit (and ROA, when the entry specifies one). Inference only happens when
there is a clear winner (≥3 observations, a majority share, and ≥2× the
runner-up); otherwise the entry is left unresolved and a warning is logged.
When a substance is inferred, the dominant ROA for that unit+substance is
filled in if the entry didn't specify one. Inferred entries are flagged with
implicit_substance: True.Related parser fixes surfaced by this change
hit/hitsas units (matching the existing dose-unit definitions indose.py), so they're no longer parsed as a substance.(~5%).(10:1)before attempting to parse a dose, so thenow-optional substance doesn't turn them into phantom subdoses.
Test plan
pytest— 53 passed (7 new: unit-only parsing, approx-percent note, andinference behavior incl. clear-winner / ambiguous / below-threshold /
explicit-not-overwritten)
mypy --ignore-missing-import --check-untyped-defs— cleanpreviously-mishandled unit-only entries are now recovered with inferred
substances + ROAs