-
Notifications
You must be signed in to change notification settings - Fork 3
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
WSG-99 Indicators HIV.IND.37-HIV.IND.45 and Refactoring of Indicator Logic #43
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of remarks, mostly aimed at the conversation later today.
and C.code ~ Concepts."HIV-positive - HIV.B.DE116" | ||
sort by start of onset.toInterval() | ||
|
||
define "First HIV Treatment": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the difference between "First HIV Treatment" and "First On ART"?
code "HIV-positive": 'HIV.B.DE34' from "HIVConcepts" display 'HIV-positive' | ||
code "HIV-negative": 'HIV.B.DE35' from "HIVConcepts" display 'HIV-negative' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't we have two HIV-positive, i.e., one in HIV.B and one in HIV.D?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm... looking at it (and this may be hard to do in a principled way) HIV.B.DE33
(the question to which these are the implied answers) concerns partner HIV status whereas HIV.B.DE115
concerns the client's HIV status. Of the two, it's probably less confusing if HIV.B.DE116
and HIV.B.DE117
are the plain "HIV-positive" and "HIV-negative" concepts.
input/cql/HIVElements.cql
Outdated
|
||
parameter "Measurement Period" Interval<Date> default Interval[@2020-01-01, @2020-12-31] | ||
// parameter "Measurement Period" Interval<Date> default Interval[@2020-01-01, @2020-12-31] | ||
parameter "Measurement Date" Date default @2020-01-01 | ||
parameter "Testing Interval" System.Quantity default 3 months |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This parameter is a little weird. I think 3 month re-testing is standard for clients on PrEP, but, e.g., for clients on ART who aren't experiencing treatment failure, its generally annual testing and the WHO recommendations are basically annual retests for people at high risk with 3-6 month re-tests "based on individual risk factors".
Maybe it could be renamed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks great! A couple of concerns about how we're modelling things. Specifically, I'm not quite convinced that the DiagnosticReport and ServiceRequest for HIV Tests are really adding values above an Observation-based model, since the concern is
input/cql/HIVElements.cql
Outdated
define "HIV test result DiagnosticReport": | ||
[DiagnosticReport: Concepts."HIV test type - HIV.B.DE81 Choices"] DR | ||
with "HIV test" SR | ||
such that DR.basedOn.references(SR) | ||
where DR.status in { 'final', 'amended', 'corrected' } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really want to make this dependent on a DiagnosticReport? In this case, it's not clear to me what value the DR adds over the Observation for our purposes (i.e., the Observation still has the basedOn
stuff, we're only really checking a single observation, etc.
input/cql/HIVElements.cql
Outdated
define "HIV test": | ||
[ServiceRequest: Concepts."HIV test type - HIV.B.DE81 Choices"] SR | ||
where SR.status in { 'active', 'completed' } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit curious about this mapping:
- The service request is, as the name implies, a request and not an actual record of a test performed. Unless we're specifically trying to calculate how many tests were ordered its not clear to me that this is semantically the correct resource to use.
- It makes the "Non-Self HIV test" look (to me) like we're creating a resource that's just their to structurally allow the ServiceRequest to function. HIV self-tests are available on a non-prescription basis, which means that in the normal course of things, you'd only ever have orders for a small sub-set of HIV tests.
The testing algorithm in B7.DT does create ServiceRequests, but the actual record of the test being done, etc. but the final result is always recorded as an algorithm.
input/cql/HIVElements.cql
Outdated
|
||
define "HIV test result DiagnosticReport Value": | ||
"HIV test result DiagnosticReport Observation" O | ||
return O.value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be worth considering using interpretation
vs value
here, but I don't feel very strongly about that.
e64f51b
to
908fded
Compare
No description provided.