Personal/wiy/support observation category #26

wi-y · 2020-01-15T21:41:09Z

added support for observation category

LiquidPT · 2020-01-15T21:45:10Z

src/lib/Microsoft.Health.Fhir.Ingest/Template/CodeValueFhirTemplate.cs

@@ -9,6 +9,10 @@ namespace Microsoft.Health.Fhir.Ingest.Template
 {
    public class CodeValueFhirTemplate : FhirTemplate
    {
+#pragma warning disable CA2227
+        public virtual IList<FhirCodeableConcept> Category { get; set; }


Should this be IList like the "Codes" property below?

Oh, I see, Codes is a singular CodeableConcept (list of codes), whereas Category is a list of CodeableConcepts: https://www.hl7.org/fhir/observation.html

Yeap! Confusing at first but it makes sense. It is something Will and I talked through today. For codes it is singular codeable concept because it is still one concept but there could be different code systems (LOINC, SNOMED, etc). Categories is different. You could have two different concepts say something is a vital sign as well as a iomt.

LiquidPT · 2020-01-15T21:52:58Z

src/lib/Microsoft.Health.Fhir.R4.Ingest/Template/CodeValueFhirTemplateProcessor.cs

@@ -81,6 +86,12 @@ protected override Observation MergeObservationImpl(CodeValueFhirTemplate templa

            existingObservation.Status = ObservationStatus.Amended;

+            existingObservation.Category = null;


Do we want to eliminate the categories on the existing observation? Are we assuming that the only ones there are the ones from our template?

It is an assumption the connector "owns" the observations it creates. As a side note I don't think that is described in our documentation so we should add that where appropriate. On to the behavior I would prefer a union of the categories but this also works. A union would be problematic with the current data we have since it is an array of array and the intersection isn't entirely clear. It would also open us up to weird cases like if the template had the same codes duplicated across multiple categories in the array.

The question becomes if a template is changed while an observation is in progress what do we want to happen? Right now the categories from the latest template win, so if it was vital signs but now just iomt, then iomt will be what is in the final observation. This is slightly different from how we handle codes where we just append what is in the template but never remove what isn't found. I would prefer the logic match but complexity is different for this scenario. I am okay with this approach if the rest of the team is.

It's up for debate, but my preference is to overwrite.

To me it seems strange to have one system sending in a category for an observation and then have another system send in another category for the same observation. I would assume that both systems would be consistent and send the same category for the same observation.

I don't mind pulling out this particular line of code though. If no category is specified in the template then this line wipes out the category if it previously existed in the observation, which is a bit controversial.

If we remove this line then we only overwrite if there is a category specified in the template. However, as a minor side effect, there would be no way to delete a category.

Thoughts?

I am fine keeping it. Wiping out is fine since if that is the template change there won't be a category for observations going forward.

So just to make sure the only way this changes is template changes, so overwrite going forward is OK with me and we would have to assume some consistency due diligence on the template managers part. Also if it runs through a profile on the way in that could also help with enforcement/consistency.

@sordahl-ga through the connector/tool that is correct. Changing the template is the only way to make modifications. If an organization allowed observations created by the connector to be edited by others there could be an issue but our position is the connector should own the observations it creates and they aren't mutable by other actors.

@dustinburson I was thinking of a use case (and maybe this is an incorrect usage of category) where a system would analyze observations and flag them for some reason (possibly for further investigation, or it's out of the norm or some other reason). Our system would then wipe that out if another reading in that time window comes in.

@LiquidPT The scenario makes sense but I don't think that is an appropriate use of category according to the FHIR spec. More appropriate would be using another observation that references the device one with an interpretation or a risk assessment. If the underlying observation needs to be modified for the purpose you describe using the interpretation property (https://www.hl7.org/fhir/observation-definitions.html#Observation.interpretation) or an extension seems more appropriate.

LiquidPT · 2020-01-15T21:56:12Z

test/Microsoft.Health.Fhir.R4.Ingest.UnitTests/Template/CodeValueFhirTemplateProcessorTests.cs

@@ -542,5 +542,60 @@ public void GivenTemplateWithComponentAndObservationWithOutComponent_WhenMergObs
                        && v.end == boundary.end
                        && v.values.First().Item2 == "v2"));
        }
+
+        [Fact]
+        public void GivenTemplateWithCategory_ThenCategoryReturned()


Maybe add another test where we merge observations with categories? Would help us solidify what we expect from that and ensure it's working as expected.

Agreed another test for the merge should be added. Please also cover the case of that existing codes are replaced during the merge. This will help establish that the behavior is intended.

LiquidPT · 2020-01-15T21:58:08Z

test/Microsoft.Health.Fhir.R4.Ingest.UnitTests/Template/CodeValueFhirTemplateProcessorTests.cs

+            Assert.Equal("a", observation.Category[0].Coding[0].Code);
+            Assert.Equal("b", observation.Category[0].Coding[0].Display);
+            Assert.Equal("c", observation.Category[0].Coding[0].System);
+            Assert.Equal("category text 1", observation.Category[0].Text);


Should there not be a second coding (d;e;f) in here to test for?

Good catch. Will update the test

dustinburson · 2020-01-15T22:11:12Z

docs/Configuration.md

@@ -322,6 +322,7 @@ The CodeValueFhirTemplate is currently the only template supported in FHIR mappi
 | --- | ---
 |**TypeName**| The type of measurement this template should bind to. There should be at least one DeviceContent template that outputs this type.
 |**PeriodInterval**|The period of time the observation created should represent. Supported values are 0 (an instance), 60 (an hour), 1440 (a day).
+|**Category**|Any number of [Codings](http://hl7.org/fhir/datatypes-definitions.html#coding) to classify the type of observation created.


Isn't more accurate to say any number of codeable concepts and link the codeable concept definition?

Yeah, I agree

dustinburson · 2020-01-15T22:37:48Z

src/lib/Microsoft.Health.Fhir.R4.Ingest/Template/CodeValueFhirTemplateProcessor.cs

@@ -81,6 +86,12 @@ protected override Observation MergeObservationImpl(CodeValueFhirTemplate templa

            existingObservation.Status = ObservationStatus.Amended;

+            existingObservation.Category = null;


It is an assumption the connector "owns" the observations it creates. As a side note I don't think that is described in our documentation so we should add that where appropriate. On to the behavior I would prefer a union of the categories but this also works. A union would be problematic with the current data we have since it is an array of array and the intersection isn't entirely clear. It would also open us up to weird cases like if the template had the same codes duplicated across multiple categories in the array.

The question becomes if a template is changed while an observation is in progress what do we want to happen? Right now the categories from the latest template win, so if it was vital signs but now just iomt, then iomt will be what is in the final observation. This is slightly different from how we handle codes where we just append what is in the template but never remove what isn't found. I would prefer the logic match but complexity is different for this scenario. I am okay with this approach if the rest of the team is.

dustinburson · 2020-01-15T22:39:29Z

test/Microsoft.Health.Fhir.R4.Ingest.UnitTests/Template/CodeValueFhirTemplateProcessorTests.cs

@@ -542,5 +542,60 @@ public void GivenTemplateWithComponentAndObservationWithOutComponent_WhenMergObs
                        && v.end == boundary.end
                        && v.values.First().Item2 == "v2"));
        }
+
+        [Fact]
+        public void GivenTemplateWithCategory_ThenCategoryReturned()


Agreed another test for the merge should be added. Please also cover the case of that existing codes are replaced during the merge. This will help establish that the behavior is intended.

test/Microsoft.Health.Fhir.R4.Ingest.UnitTests/Template/CodeValueFhirTemplateProcessorTests.cs

dustinburson · 2020-01-15T22:48:02Z

test/Microsoft.Health.Fhir.R4.Ingest.UnitTests/Template/CodeValueFhirTemplateProcessorTests.cs

+
+            var observation = processor.CreateObservation(template, observationGroup);
+            Assert.Equal("a", observation.Category[0].Coding[0].Code);
+            Assert.Equal("b", observation.Category[0].Coding[0].Display);


Can use the Assert.Collection() function to iterate through a collection and then supply lambda's for each collection index. Results in a little cleaner syntax and better error messages in the test output then things aren't correct. There are examples in other tests here like GivenTemplateWithComponent_WhenCreateObservation_ThenObservationReturned_Test.

dustinburson · 2020-01-15T23:23:42Z

src/lib/Microsoft.Health.Fhir.R4.Ingest/Template/CodeValueFhirTemplateProcessor.cs

+                new CodeableConcept
+                {
+                    Text = category.Text,
+                    Coding = category.Codes.Select(code => new Coding { System = code.System, Code = code.Code, Display = code.Display })


Will want to make sure a text only category but null codings works correctly as well. I believe this code will error if the category codes are null.

wi-y

updated code/tests to address feedback

wi-y added 3 commits January 15, 2020 13:35

support observation category

2e05cc4

add observation category test

25db01c

documentation update for category

919dab6

wi-y requested review from LiquidPT, namalu and dustinburson January 15, 2020 21:41

LiquidPT reviewed Jan 15, 2020

View reviewed changes

dustinburson reviewed Jan 15, 2020

View reviewed changes

dustinburson requested a review from sordahl-ga January 15, 2020 23:24

wi-y added 3 commits January 16, 2020 11:32

correct the documentation for category

114f719

tests for category during merged observation

51bc633

check for null category codes

d792c91

wi-y requested review from LiquidPT and dustinburson January 16, 2020 20:37

wi-y commented Jan 16, 2020

View reviewed changes

LiquidPT approved these changes Jan 16, 2020

View reviewed changes

dustinburson approved these changes Jan 17, 2020

View reviewed changes

wi-y merged commit 9978ef3 into master Jan 17, 2020

dustinburson deleted the personal/wiy/support-observation-category branch November 8, 2022 18:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Personal/wiy/support observation category #26

Personal/wiy/support observation category #26

wi-y commented Jan 15, 2020

LiquidPT Jan 15, 2020

LiquidPT Jan 15, 2020

dustinburson Jan 15, 2020

LiquidPT Jan 15, 2020

dustinburson Jan 15, 2020

wi-y Jan 15, 2020

dustinburson Jan 15, 2020

sordahl-ga Jan 16, 2020

dustinburson Jan 16, 2020

LiquidPT Jan 16, 2020

dustinburson Jan 17, 2020

LiquidPT Jan 15, 2020

dustinburson Jan 15, 2020

LiquidPT Jan 15, 2020

wi-y Jan 15, 2020

dustinburson Jan 15, 2020

wi-y Jan 16, 2020

dustinburson Jan 15, 2020

dustinburson Jan 15, 2020

dustinburson Jan 15, 2020

dustinburson Jan 15, 2020

wi-y left a comment

		@@ -81,6 +86,12 @@ protected override Observation MergeObservationImpl(CodeValueFhirTemplate templa

		existingObservation.Status = ObservationStatus.Amended;

		existingObservation.Category = null;

Personal/wiy/support observation category #26

Personal/wiy/support observation category #26

Conversation

wi-y commented Jan 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wi-y left a comment

Choose a reason for hiding this comment