Skip to content
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

Add variable Extension support #1059

Closed
fredhersch opened this issue Jan 21, 2022 · 23 comments · Fixed by #1328
Closed

Add variable Extension support #1059

fredhersch opened this issue Jan 21, 2022 · 23 comments · Fixed by #1328
Assignees
Labels
effort:medium Medium effort - 3 to 5 days P2 Medium priority issue

Comments

@fredhersch
Copy link
Collaborator

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)

  • From the current Questionnaire, it is simpler to set a variable and then use that in the calculation. See example here

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

@Tarun-Bhardwaj Tarun-Bhardwaj added Triage P3 Low priority issue effort:medium Medium effort - 3 to 5 days and removed Triage labels Jan 28, 2022
@shoaibmushtaq25 shoaibmushtaq25 self-assigned this Mar 8, 2022
@shoaibmushtaq25
Copy link
Contributor

shoaibmushtaq25 commented Mar 14, 2022

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
Copy link
Collaborator

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?

@jingtang10 jingtang10 added P2 Medium priority issue and removed P3 Low priority issue labels Mar 18, 2022
@shoaibmushtaq25
Copy link
Contributor

shoaibmushtaq25 commented Mar 18, 2022

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
Copy link
Contributor

@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
Copy link
Contributor

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
Copy link
Collaborator

@ekigamba @joiskash @maimoonak fyi

@shoaibmushtaq25
Copy link
Contributor

shoaibmushtaq25 commented Mar 31, 2022

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
Copy link
Contributor

shoaibmushtaq25 commented Apr 1, 2022

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
Copy link
Collaborator

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

@joiskash
Copy link
Collaborator

joiskash commented Apr 1, 2022

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
Copy link
Contributor

shoaibmushtaq25 commented Apr 1, 2022

@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
Copy link
Contributor

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
Copy link
Collaborator

joiskash commented Apr 4, 2022

@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..

@google google deleted a comment from joiskash Apr 4, 2022
@shoaibmushtaq25
Copy link
Contributor

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

@shoaibmushtaq25
Copy link
Contributor

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
Copy link
Contributor

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

@shoaibmushtaq25
Copy link
Contributor

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
Copy link
Contributor

shoaibmushtaq25 commented Jun 20, 2022

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
Copy link
Contributor

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

@shoaibmushtaq25
Copy link
Contributor

shoaibmushtaq25 commented Jun 29, 2022

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
Copy link
Contributor

shoaibmushtaq25 commented Jul 21, 2022

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
Copy link
Collaborator Author

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

@fredhersch fredhersch moved this to In Progress in Android FHIR SDK Aug 30, 2022
@shoaibmushtaq25
Copy link
Contributor

shoaibmushtaq25 commented Aug 30, 2022

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

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

Repository owner moved this from In Progress to Complete in Android FHIR SDK Sep 1, 2022
@Tarun-Bhardwaj Tarun-Bhardwaj moved this to Backlog in Android FHIR SDK Sep 16, 2022
@Tarun-Bhardwaj Tarun-Bhardwaj moved this from Backlog to Complete in Android FHIR SDK Sep 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort:medium Medium effort - 3 to 5 days P2 Medium priority issue
Projects
Status: Complete
6 participants