google / android-fhir

The Android FHIR SDK is a set of Kotlin libraries for building offline-capable, mobile-first healthcare applications using the HL7® FHIR® standard on Android.
https://google.github.io/android-fhir/
Apache License 2.0
463 stars 241 forks source link

Add variable Extension support #1059

Closed fredhersch closed 1 year ago

fredhersch commented 2 years ago

Is your feature request related to a problem? Please describe. Desire to use variable names to perform calculations via SDC.

For example, to calculate BMI: (weight_value/heigh_value*height_value)

Describe the solution you'd like Implement variable Extension support - see specification

shoaibmushtaq25 commented 2 years ago

The proposed solution is :

In the extraction part in ResourceMapper, we can evaluate the expression of the named variable (such as weight) in this example, and then use this evaluated value of the named variable in the subsequent usages of this variable(such as %weight) as given in this example. Assuming that FHIRPath supports the evaluation of where clause expression mentioned here ("%resource.repeat(item).where(linkId='3.3.1').item.answer.valueDecimal"), we can use the evaluate value on subsequent usages (%weight)

Let me know if this approach is workable or suggest me better alternatives, I am open to other suggestions. Thanks @jingtang10 @Tarun-Bhardwaj cc: @f-odhiambo @ndegwamartin

jingtang10 commented 2 years ago

my first question is that do you evaluate the expressions lazily or do you evaluate them before hand?

do you cache the values? and if you do how do you update them when the questionnaire answers change?

shoaibmushtaq25 commented 2 years ago

As per discussion with @jingtang10 over the call, First, I will explore whether FHIRPath supports the variables to pass them as input parameters to evaluate an expression. If I wouldn't be able to find it, I should ask a question over the Zulip FHIR chat to get an opinion from the community experts.

Let's say, if FHIR Path supports the variables to pass them as input parameters to evaluate an expression, then we need to evaluate the variable values before hand and then pass these values to the FHIR path to evaluate further expression (i.e, BMI calculation)

Also, I have to explore that does this variable used in the SDC population or not. @fredhersch Can you please share some examples of usage of variables in the population if there is any possibility to use it in population.

@jingtang10 Can you please review my comment and let me know If I missed something here, Thanks. cc: @jingtang10 @f-odhiambo

shoaibmushtaq25 commented 2 years ago

@jingtang10 , I posted this query over Zulip FHIR Chat and got an answer here. As per the comment, It should work in FHIR Path, I am going to try this out how can we apply this using FHIR Path.

shoaibmushtaq25 commented 2 years ago

I tried to evaluate the fhirpath expressions like %resource.repeat(item).where(linkId='3.3.1').item.answer.valueDecimal as mentioned in the questionnaire here and this worked well.

The solution, I am planning to implement is, Let's say, we calculate the value of variable based on the expression e,g %resource.repeat(item).where(linkId='3.3.1').item.answer.valueDecimal mentioned on the extension at the root of questionnaire/questionnaireItem and save this value in the related QuestionnaireResponse (extension and its value).

Whenever the value of this variable changes (Let's say the user enters the new value of weight/height), we will update this value in the related questionnaireResponse and we will be able to use the value of this variable where needed in the questionnaire in the descendants.

One thing to think over that does these variables have their usage in the population or not and if we have, please share some related examples of it, It would be helpful to cover those cases as well. @jingtang10 cc: @f-odhiambo

jingtang10 commented 2 years ago

@ekigamba @joiskash @maimoonak fyi

shoaibmushtaq25 commented 2 years ago

The other approach that was suggested by @jingtang10 (please correct me If I wrote something wrong here) in the SDK Dev call is that, We can evaluate the value of variable expression where it's needed (Calculate value on-demand). Let's say, in the questionnaire here, the variables %weight and %height are used in the calculation of BMI with calculated expression.

The question: When the value of the variable(width/height) changes after user input, how do we calculate the value of the occurrences of Variables where they are used (for example, in the calculated expression)? Do we add some callback to listen this change and update the value of the variable accordingly?

Note that we have a separate ticket for CalculatedExpression as well.

Variable extensions with expressions are here, "extension" : [ { "url" : "http://hl7.org/fhir/StructureDefinition/variable", "valueExpression" : { "name" : "weight", "language" : "text/fhirpath", "expression" : "%resource.repeat(item).where(linkId='3.3.1').item.answer.valueDecimal" } }, { "url" : "http://hl7.org/fhir/StructureDefinition/variable", "valueExpression" : { "name" : "height", "language" : "text/fhirpath", "expression" : "%resource.repeat(item).where(linkId='3.3.2').item.answer.valueDecimal*0.0254" } ]

The usage of these variables is in the calculation of BMI (with Calculated Expression) is given below, { "extension": [ { "url": "http://hl7.org/fhir/uv/sdc/StructureDefinition/sdc-questionnaire-calculatedExpression", "valueExpression": { "description": "BMI Calculation", "language": "text/fhirpath", "expression": "(%weight/(%height.power(2))).round(1)" } } ], "linkId": "3.3.3", "text": "Your Body Mass Index (BMI) is", "type": "decimal", "readOnly": true }

Please share your thoughts on these approaches or/and feel free to suggest a better approach to support variable extension. @jingtang10 @ekigamba @maimoonak @joiskash cc: @f-odhiambo

shoaibmushtaq25 commented 2 years ago

Moreover, I asked a question over zulip chat here related to power() function (%weight/(%height.power(2))).round(1) which wasn't evaluated correctly with fhirPathEngine and gives an error around "power is not a valid function". The conversation around it still going on over zulip chat. cc: @jingtang10 @ekigamba @maimoonak

f-odhiambo commented 2 years ago

@Tarun-Bhardwaj kindly let us know if we can proceed with suggested above approach

joiskash commented 2 years ago

The other approach that was suggested by @jingtang10 (please correct me If I wrote something wrong here) in the SDK Dev call is that, We can evaluate the value of variable expression where it's needed (Calculate value on-demand). Let's say, in the questionnaire here, the variables %weight and %height are used in the calculation of BMI with calculated expression.

The question: When the value of the variable(width/height) changes after user input, how do we calculate the value of the occurrences of Variables where they are used (for example, in the calculated expression)? Do we add some callback to listen this change and update the value of the variable accordingly?

Note that we have a separate ticket for CalculatedExpression as well.

Variable extensions with expressions are here, "extension" : [ { "url" : "http://hl7.org/fhir/StructureDefinition/variable", "valueExpression" : { "name" : "weight", "language" : "text/fhirpath", "expression" : "%resource.repeat(item).where(linkId='3.3.1').item.answer.valueDecimal" } }, { "url" : "http://hl7.org/fhir/StructureDefinition/variable", "valueExpression" : { "name" : "height", "language" : "text/fhirpath", "expression" : "%resource.repeat(item).where(linkId='3.3.2').item.answer.valueDecimal*0.0254" } ]

The usage of these variables is in the calculation of BMI (with Calculated Expression) is given below, { "extension": [ { "url": "http://hl7.org/fhir/uv/sdc/StructureDefinition/sdc-questionnaire-calculatedExpression", "valueExpression": { "description": "BMI Calculation", "language": "text/fhirpath", "expression": "(%weight/(%height.power(2))).round(1)" } } ], "linkId": "3.3.3", "text": "Your Body Mass Index (BMI) is", "type": "decimal", "readOnly": true }

Please share your thoughts on these approaches or/and feel free to suggest a better approach to support variable extension. @jingtang10 @ekigamba @maimoonak @joiskash cc: @f-odhiambo

Just some initial thoughts on how this should be implemented. Please criticize it. Seems like a map or some other similar data structure should be created while parsing the questionnaire and creating the questionnaire response as an index to map linkIds and their dependencies. So in the example above 3.3.3 would depend on 3.3.2 & 3.3.1. When either one of the dependencies change 3.3.3 would be computed again. All this can be triggered in the questionnaireResponseItemChangedCallback. One point that I want to bring up is that the current assumption that questionnaireResponse.item can be uniquely identified with linkIds is incorrect and breaks when nested items are repeated. You can find more information on this here .

shoaibmushtaq25 commented 2 years ago

@joiskash Thanks for your input on this. I also suggested another approach above to not calculate the value lazily but beforehand and update it once the user enters the new value. Can you please review this approach too and give your input on that?

shoaibmushtaq25 commented 2 years ago

The other approach that was suggested by @jingtang10 (please correct me If I wrote something wrong here) in the SDK Dev call is that, We can evaluate the value of variable expression where it's needed (Calculate value on-demand). Let's say, in the questionnaire here, the variables %weight and %height are used in the calculation of BMI with calculated expression. The question: When the value of the variable(width/height) changes after user input, how do we calculate the value of the occurrences of Variables where they are used (for example, in the calculated expression)? Do we add some callback to listen this change and update the value of the variable accordingly? Note that we have a separate ticket for CalculatedExpression as well. Variable extensions with expressions are here, "extension" : [ { "url" : "http://hl7.org/fhir/StructureDefinition/variable", "valueExpression" : { "name" : "weight", "language" : "text/fhirpath", "expression" : "%resource.repeat(item).where(linkId='3.3.1').item.answer.valueDecimal" } }, { "url" : "http://hl7.org/fhir/StructureDefinition/variable", "valueExpression" : { "name" : "height", "language" : "text/fhirpath", "expression" : "%resource.repeat(item).where(linkId='3.3.2').item.answer.valueDecimal*0.0254" } ] The usage of these variables is in the calculation of BMI (with Calculated Expression) is given below, { "extension": [ { "url": "http://hl7.org/fhir/uv/sdc/StructureDefinition/sdc-questionnaire-calculatedExpression", "valueExpression": { "description": "BMI Calculation", "language": "text/fhirpath", "expression": "(%weight/(%height.power(2))).round(1)" } } ], "linkId": "3.3.3", "text": "Your Body Mass Index (BMI) is", "type": "decimal", "readOnly": true } Please share your thoughts on these approaches or/and feel free to suggest a better approach to support variable extension. @jingtang10 @ekigamba @maimoonak @joiskash cc: @f-odhiambo

Just some initial thoughts on how this should be implemented. Please criticize it. Seems like a map or some other similar data structure should be created while parsing the questionnaire and creating the questionnaire response as an index to map linkIds and their dependencies. So in the example above 3.3.3 would depend on 3.3.2 & 3.3.1. When either one of the dependencies change 3.3.3 would be computed again. All this can be triggered in the questionnaireResponseItemChangedCallback. One point that I want to bring up is that the current assumption that questionnaireResponse.item can be uniquely identified with linkIds is incorrect and breaks when nested items are repeated. You can find more information on this here .

@joiskash , I see your point and checked the related open issue#910 around it and yeah the assumption (questionnaireResponse.item can be uniquely identified with linkIds) would break when nested items are repeated. so right now, we don't have support for these types of questionnaire items (nested item with repeat). What do you think if we first support variables based on linkIds and later on we can support the other use cases when we have rendering support available for those questionnaire items(nested items with repeat) after issue#910 gets merged.

joiskash commented 2 years ago

@joiskash Thanks for your input on this. I also suggested another approach above to not calculate the value lazily but beforehand and update it once the user enters the new value. Can you please review this approach too and give your input on that?

Sorry, Im not sure I understand the exact difference between the two approaches..

shoaibmushtaq25 commented 2 years ago

I have asked a question about how to pass variables as input to fhirPathEngine evaluate method, Got a reply on that here.

shoaibmushtaq25 commented 2 years ago

The SDC Specification said that,

Variable specifying a logic to generate a variable for use in subsequent logic. The name of the variable will be added to FHIRPath's context when processing descendants of the element that contains this extension.

so what exactly is the meaning of "adding the name of the variable to the FHIRPath's context" here? Can anyone help me understand it better?

My understanding of it is that we iterate through the descendants of the questionnaireItem on which the variable extension is defined and add these extensions with the evaluated value of variables into the respective questionnaireResponseItems, we can then use these variable values where needed in the expressions but still I am confused on FHIRPath's context part, how we will add them in the FHIRPath's context and what exactly it means?

@jingtang10 @joiskash @ekigamba @maimoonak cc: @f-odhiambo

shoaibmushtaq25 commented 2 years ago

I asked a query over FHIR zulip chat around variable definition at the questionnaire root level , got a reply here

shoaibmushtaq25 commented 1 year ago

Based on Jing’s feedback in a call, I am working on a solution to

  1. Find out dependent variables after building some sort of dependencies map of variables
  2. When a variable gets updated, instead of updating all the variables, update only the dependent variables
  3. Test all the cases to make sure all are working fine, add failing and new test cases

Out of these 3, I am done with Step 1 (PR#1328 is updated with it) and now working on Step 2 and then Step 3.

cc: @f-odhiambo @maimoonak @jingtang10

shoaibmushtaq25 commented 1 year ago

The PR #1328 is updated with step 1, step 2 and step 3(partially, need to work on unit tests) of the solution mentioned above. cc: @maimoonak @f-odhiambo

shoaibmushtaq25 commented 1 year ago

All three steps completed, PR#1328 is ready for review by Google team.

shoaibmushtaq25 commented 1 year ago

Summary of Implemented Solution on PR#1328:

  1. Firstly, we prepare a map of linkIds to QuestionnaireItemPath Screenshot from 2022-06-29 13-28-59

  2. Secondly, we prepare a map of questionnaireItemPath to variables + root path to variables Screenshot from 2022-06-29 13-36-59

  3. In the QuestionnaireState here, We check the variable expressions with Regex to find out the dependents of variables and prepare a map of questionniareItemPath.VariableName to DependentOfs (on which the value of a variable depends on) Screenshot from 2022-06-29 14-25-34 For example, In the following extension, root variable X depends on (variableY + answer of item with linkId = fieldAB), the map entry be like "/.X" -> [variableY, fieldAB] { "url": "http://hl7.org/fhir/StructureDefinition/variable", "valueExpression": { "name": "X", "language": "text/fhirpath", "expression": "%variableY + %resource.repeat(item).where(linkId='fieldAB').answer.first().value" } }

  4. In the QuestionnaireState here, we are trying to calculate all the variables(root level, group level, item level) so that the variables having simple expressions and are not dependent on any other variables/answer values gets their value calculated.

  5. Call updateDependentVariables on the fly whenever questionnaireResponseItemChangedCallback triggers on answer change of any questionnaire item when the user interacts with. In updateDependentVariables, we check in the map that if any variable depends on the given linkId/variable as a predicate, calculate the value of that variable. we do this step recursively until there is no variable left which is dependent of given linkId/variable as a predicate.

cc: @f-odhiambo @jingtang10 @maimoonak

shoaibmushtaq25 commented 1 year ago

After removal of linkIdToQuestionnaireItemMap / linkIdToQuestionnaireResponseMap by @jingtang10 in the PR#1468 and he wrote a different solution in PR#1496 to alternatively trace questionnaireItems and questionnaireResponseItems, I started to align my solution of variable extension support in PR#1328 accordingly as suggested by Jing here PR#1328 comment because my current solution was strongly relying on linkIdToQuestionnaireItemMap and linkIdToQuestionnaireResponseItemMap.

The new approach that I am going to implement now is,

  1. Make a child-to-parent map for QuestionnaireItems as we have it for QuestionnaireResponseItems because we need to go through the hierarchy upwards to trace/evaluate and put the values of variables down in the descendants.
  2. Make a method evaluateExpression() which would take questionnaireItem and expression and return the evaluated answer of the expression. In this method, we will try to calculate/evaluate an expression, if it depends on other variables which probably defined up in the hierarchy, we recursively go through the hierarchy with the help of the child-parent map prepared in step 1 above and get the value of the variable and put it's value into original expression, evaluate the expression and return the result. We will calculate variables recursively on the fly and not storing/calculating variables beforehand.

cc: @jingtang10 @Tarun-Bhardwaj @maimoonak @f-odhiambo

fredhersch commented 1 year ago

@shoaibmushtaq25 @Tarun-Bhardwaj what's the status of this issue?

shoaibmushtaq25 commented 1 year ago

@shoaibmushtaq25 @Tarun-Bhardwaj what's the status of this issue?

Hey @fredhersch , PR just approved. Going to merge it hopefully by today.