-
Notifications
You must be signed in to change notification settings - Fork 336
(fix) O3-5309: Use observation-level reference ranges for trendline chart and display #2926
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
base: main
Are you sure you want to change the base?
(fix) O3-5309: Use observation-level reference ranges for trendline chart and display #2926
Conversation
|
Hey @BlessedAmrita, looks good to me per the attached screenshots. Kindly add Ian and Dennis as the reviewers |
|
@denniskigen @ibacher Please review this PR, thanks! |
packages/esm-patient-tests-app/src/test-results/trendline/trendline-resource.tsx
Outdated
Show resolved
Hide resolved
|
It'd also be great to add some coverage for // trendline-resource.test.tsx
import { type OBSERVATION_INTERPRETATION } from '@openmrs/esm-patient-common-lib';
import { type TreeNode } from '../filter/filter-types';
import { computeTrendlineData } from './trendline-resource';
describe('computeTrendlineData', () => {
it('propagates observation-level reference ranges to the returned node', () => {
const tree: TreeNode = {
display: 'Root',
flatName: 'Root',
subSets: [
{
display: 'Hemoglobin',
flatName: 'Hemoglobin',
units: 'g/dL',
hiNormal: 9,
lowNormal: 5,
obs: [
{
obsDatetime: '2022-05-01T00:00:00.000Z',
value: '8',
interpretation: 'NORMAL' as OBSERVATION_INTERPRETATION,
lowNormal: 10,
hiNormal: 12,
},
{
obsDatetime: '2023-05-01T00:00:00.000Z',
value: '9',
interpretation: 'NORMAL' as OBSERVATION_INTERPRETATION,
lowNormal: 12,
hiNormal: 16,
},
],
},
],
} as TreeNode;
const [node] = computeTrendlineData(tree);
expect(node.range).toBe('12 – 16 g/dL');
expect(node.lowNormal).toBe(12);
expect(node.hiNormal).toBe(16);
});
}); |
packages/esm-patient-tests-app/src/test-results/trendline/trendline-resource.tsx
Outdated
Show resolved
Hide resolved
|
I think that this ticket may need some more thought before we jump to implementing this. The problem is that I don't know that there's a good mechanism to determine the reference range for a time-series of values, and that's because each observation in the time series could have a separate reference range depending on various factors like:
Because of these sort of factors, while it's easy to put together a range for the individual observation, it's not really possible to sensibly do this with a set of observations unless they all happen to have the same reference range. The long and the short of it is that I think the more correct thing to do is to remove the attempt to display a range here at all and simply let the trends speak for themselves. |
e871816 to
fc03cac
Compare
|
I've updated this PR to align with the current app logic per Dennis's review, but I'm happy to defer to whatever you both decide regarding the feature's direction! |
packages/esm-patient-tests-app/src/test-results/trendline/trendline-resource.tsx
Outdated
Show resolved
Hide resolved
packages/esm-patient-tests-app/src/test-results/trendline/trendline-resource.tsx
Outdated
Show resolved
Hide resolved
packages/esm-patient-tests-app/src/test-results/trendline/trendline-resource.tsx
Outdated
Show resolved
Hide resolved
|
Agreed. Because the backend snapshots whatever criteria-based range happens to be active when each observation is saved, you can’t reliably derive a single “true” range covering an entire time series—those per-observation ranges reflect changing patient context (pregnancy status, equipment, new lab calibration) and even data-entry timing. The safest, least misleading option is to stop presenting a global range in the trendline header and instead let the per-point interpretations (color/tooltip) speak for themselves. That way we avoid implying a stable corridor that may never have existed and eliminate the risk of comparing apples to oranges when the underlying ranges differ. Also, once we do get the tooltips wired up with the per-value reference ranges, it probably makes sense to also add a column to the Results table at the bottom of the modal including those per-value ranges. |
|
Also, should I update the test file to align with this new logic (i.e. using the concept-level ranges for the header)...? From: expect(node.range).toBe('12 – 16 g/dL');
expect(node.lowNormal).toBe(12);
expect(node.hiNormal).toBe(16);To: expect(node.range).toBe('5 – 9 g/dL')
expect(node.lowNormal).toBe(5);
expect(node.hiNormal).toBe(9); |

Requirements
Summary
This PR fixes the trendline to show the same reference range as the results table. It now uses the observation ref range first and in case it's not there it falls back to the concept range (as the prev code)
Screenshots
After fix:


Related Issue
https://issues.openmrs.org/browse/O3-5309
Other