Enhanced Cohort Sampling and Patient Profiles

[MaximMoinat]

Two new features:

• A new cohort sampling feature, allowing to store created samples per cohort and data source.
• Completely redesigned patient profiles; allowing focussing on particular concepts and comparing profiles between patients.

These two features are combined in one PR, because they are interdependent. From the sample tab there is link to show the profile of the selected patient(s).

Both features are explained in more detail in this forum post.

Related to OHDSI/WebAPI#1657

[chrisknoll]

Hi, I'm looking at this now, but could you look into resolving the conflict?

[chrisknoll]

Hi, Update: as I checked out your branch, I had to resolve convlicts locally for me, the conflict seems simple: just put the async refreshPrintFrindly() function before your new functions related to sample (starting with clickSampleTab())..and that seems to resolve it for me. I'm not able to merge anything back into your fork but just letting you know what I did to resolve.

[chrisknoll]

Ok, i did run into an issue related to BIGINT being sent down to Javascript. Numbers that are very large for PERSON_ID are being corrupted, for example:

CDM person_id	Javascript Person_id
802666560257341	1597144381
802666552602501	1589489541

So, when the Javascript gets the number from the server, it's corrupted and then the person_id can't be found when it navigates to the profile.

I think if these are sent to the client as a string, you may be able to avoid this. We don't do any arithmetic operations on these IDs so it should be safe to just use the value as string.

[MaximMoinat]

Thanks @chrisknoll. Will take a look at the new merge conflict and the bigint issue.

[chrisknoll]

Thanks @MaximMoinat . One more for the pile: When you pull up a sample result, the bottom table populates with the results. If you then click 'delete' on the top table, the bottom table remains. It would probably make sense to 'deselect' the sample in the bottom table if it gets deleted.

[chrisknoll]

Hi, found the following:

• Lines 30, 54 and 78 of pages/cohort-definitions/route.js: it's importing 'conceptset-list-modal' but this doesn't exist. Probably a bad merge from your master branch, but conceptsets were changed up and some components were retired. I removed those lines and it got further...
• Line 86 of pages/cohort-definitions/route.js: The call to sharedState.ConceptSet.source('cohort') no longer applies since changes to concept sets. Maybe it was a problem with merging from master, but it shouldn't be there. On my local version, I just removed that line.
• You registered a new module named 'explore-cohort' and added it to settings.js, but that's not really necessary, you can just refer to the component as components/explore-cohort. This is the preferred way since you won't really want to add every new component you create to settings.js. You can leave it if you want (since it works and resolves ok) but I think we'd prefer if you just specified the path to the component in the import statement.

I found some issues on the WebAPI side, but I'll cover those over in the other PR.

[MaximMoinat]

@chrisknoll Thanks for testing. I fixed the first two points in our branch as well. I did not fix the third point as I am not really comfortable with registering components and it works now.

[chrisknoll]

No problem. Thanks for the update, I'll check it now.

[chrisknoll]

Encountering some new errors, which is strange since previously I was able to pull up profiles, but I'm getting the following:

When loading the profile page:

Clicking 'next patient':

Could be related to the first error if something with the geometry of the D3 SVG area is off, that may lead to a negative calculation of a rectangle width, but not sure.

[chrisknoll]

Update on the first one. This is comming from line 47 of profileTimeline.js:

this.width =
        document.getElementById(this.chartContainer).offsetWidth -
        this.margin.left -
        this.margin.right

The getElementById call is returning null. I'm not sure if this is related to when you load a profile, the 'loading' indicator is showing, and I think the other dom elements are removed from the dom, so you can't find the dom eleemnts to get the width for...I can experiment locally to see what the fix is. this is very puzzling that this seemed to be working before but now running into this issue.

[chrisknoll]

Ok, more information on this:

I think part of the problem is just dealing with the component lifecycle: when the page is loaded, there's no person yet, but we want to instantiate the ProfileTimeline component. This instantiation happens once, and in the constructor of the class, it attempts to find an element which is hidden by the if: databinding when the person is null (at first, the person is null, and then loaded later). However, the compoent's width property is only initialized once, and so this gets sent some incorrect value and future 'draws' of the patent timeline fails.

Quick fix could be just to hard code the width (for now) until we work out a way to do it dynamically. There are other examples in the codebase where we render SVG (via d3) elements as charts, but they rely on databinding and other things that do not depend on 'getElementById' (something we should avoid anyway because you can't guarntee uniqueness of an element ID). But I don't want to get into a serious redesign effort here...let's see what minor changes we can make to get it to work.

[chrisknoll]

I stepped through this more, and I think the reason why i saw it working is that when you resize the browser window, a redraw() call is made which then can find the geometry of the containing div, gets an actual width value, and everything renders. I'm pretty sure with a little rework, we can get this working properly, but it's a bit hard to do so across forked branches.

So, I was going to suggest taking this PR into a Atlas-repo branch where I can make modifications, if that would be acceptable to the original authors of this PR.

[MaximMoinat]

We would be happy to have this PR taking into this Atlas-repo, if that simplifies the process. If you encounter more issues and additional testing is necessary on our side, please let us know.

[chrisknoll]

Slight change of plans on my side: I managed to fork the repo from thehyve and I'll make my own PR into your cohort-sampling branch.

I'm leaning towards doing it this way because I'd like to have a better collaboration with you guys about the proposed changes, see if you agree with it and not be surprised by anything. It didn't feel right to just absorb the branch and work on it alone...so, expect to see a PR to your fork's branch in the next couple of days.

[chrisknoll]

Hello, @MaximMoinat and The Hyve! Another day, another update:

I spent the morning reviewing the new functionality with Patrick and there is a lot of excitement and motivation to get these changes incorporated into the application. There is also an effort from the folks at Georgia Tech to incorporate a annotation feature to patient profiles that is ongoing and there's a great opportunity to collaborate together and introduced a full revamped and enhanced patient profile experience.

During the functional review, we found a number of issues/concerns that were introduced with the profile viewer update, as well as some concerns on the back end side, which I'll discuss in this comment (and not spread out feedback across 2 threads). Based on the discussion and discovery, we think that it would be possible to push a change for 2.8 that has a much more narrow focus: selecting the cohort sample from a cohort generation. Considering the other changes involved with the new Profile Viewer UI, we just don't think we can test/certify all the functionality in time for the 2.8 release.

But, the cohort sample function is relatively simple, and provides a path to the profile viewer (as a URL redirect) and I believe we can pull in the cohort sample functionality independently from the profile viewer. At this level, I can commit to getting that functionality into a 2.8 release.

Here's the (incomplete) list of items that we identify that we'd want addressed (except some of the enhancement items, but other enhancements may be required):

Bug = bad/error behavior enh = enhacement tst = need to test

Cohort Sample:

1. [bug] Shouldn't JavaScript alert box on every save.
2. [enh] Sample definitions shouldn't be deleted on every generate
3. [enh] Sample definition should be based on Circe criteria (ie: what cohort characterization uses for custom prevalence definition)
4. [tst] Need to certify that 'random sample' works properly across different DBMS. (Ie: RAND() does not exist in all of our CDM supported platforms)
5. [tst] Confirm location where patient IDs are being saved (is it a results schema table, webapi, etc?)
6. [tst] Need to confirm process works across different RDBMS to ensure that none of the column names may conflict with database keywords (ie: rank is a column in cohort_sample_elements)
7. [bug] 'rank' is being used as a column name, we are concerned about platforms not working with 'reserved' keywords in columns…to work around this we usually put _value at the end of the column so rank -> rank_value.
8. [tst] Need to confirm that this works under security enabled mode
9. [enh] sample person's 'number of events': 2 options: remove the column from the sample summary because it's slow to calculate, or: pre-compute event counts at sample time and store it in cohort_sample_elements (version 1, remove, versoin 2 cache results)
10. [enh] If we go with 'current Profile UI' but new sample function: remove ability to select multiple patients (current UI only supports single profile view)

Patient Profile Viewer

1. [bug] Component loading leads to errors due to getElementByID() returning null.
2. [bug] Need to hide the option to show the 'effective date' based on a UI configuration setting
3. [bug] The expand/collapse arrows are reversed on the domains
4. [enh] The observation period boundaries are missing
5. [enh] Prior profile viewer allowed the ability to exclude entire domains from the visualzation, that function should remain
6. [tst] Need to test the case where a person appears in the cohort multiple times (which episode is used as index day 0?)
7. [bug] long concept names are truncated on the left, but there's no 'hover' function (tooltip) to reveal the full name. As a work-around, the individual data points on the right side of the view does have tooltips with the full names, but this should be visible on the left column.
8. [enh] can the domain/concept name column in the visualization be resizable?
9. [tst] What is 'frequency' in the tooltip, and how is it calculated
10. [enh] the component should leverage databinding to associate the patient level data with the D3 visualization
11. [enh] the tabular data could provide more function: clicking on a data-row could select that concept for 'pinning'. Hiding domains on the filter could be reflected in the visualization.
12. [bug] when selecting 2 patients, the visualization fails to render in some cases
13. [enh] allow to select cohorts to add to the profile view so you can see cohort episodes in the context of the patient timeline.

Trying to cover all of these in the next 2 weeks to meet the 2.8 release is not possible, but I do think it is possible to incorporate the cohort sampling back-end function, add the new 'sample' tab to cohort definitions, and allow navigation to a patient_id from the sample results view. We still need to test this on multiple platforms (especially Impala and redshift) to ensure that it functions but we can get help from the community, and also ensure that security is configured properly.

I would like to know if you have any objections to this narrower focus. My plan would be to use the existing WebAPI pr to support the cohort sample endpoints (but we need to revise some column names of the results schema tables since I think 'rank' may be used as a reserved word on some platforms). On the Atlas side, it's a bit more complicated: I'd create a new branch in OHDSI/Atlas and I'd manually do a diff of the Hyve's atlas PR and the current Atlas master and only pull in those changes that adds the new javascript REST service calls and the new cohort definition manager tab. I'd update those tab changes to call the existing atlas URLs to load the current profile viewer. This will have a very important impact of allow the patient profile viewer to be 'accessable' without the tornado report. This is a huge win, and I think would be fairly straightforward to implement.

Thank you all for your hard work on this, and please let me know if you have any concerns. I will make sure that the proper credits are indicated in the separate commits (if you could please provide names and email addresses of people you would like me to put into the commit messages, otherwise I will credit 'The Hyve' for these contributions.

[MaximMoinat]

Again, thanks Chris (and Patrick) for this extensive review. Much appreciated that you are putting so much time and effort into making the best possible integration of these new features. Although we would really like to see both features merged for v2.8, I see no reason to split it. I will double-check with our team whether there are any objections to splitting the functionalities.

One small remark; the current sample tab offers a 'multi-person' comparison in the timelines. This has to be removed if we keep using the current profile viewer. (and this is also one of the reasons we have one branch for both features, ideally it would have been two to start with)

[chrisknoll]

@MaximMoinat ,
Thanks very much for your consideration. Yes, we understand that the current profile experience is single-person oriented, and so we'll be adjusting the 'sample result view' to simply have a table of links to the profile URL instead of checkboxes. That will be a simple change, I believe, but when your work does finally get merged in (at the next release) it will have all the supported features...I think a multi-person view is a good idea.

[chrisknoll]

Since this PR is being partially covered in #2373, I'm making this PR as a draft for now. It may be necessary to restart this PR based on the new codebase, but I leave it to you to decide how to move forward with it.