Temporal annual distribution functionality and temporal FeatureExtraction capability enablement

[alex-odysseus]

Supporting OHDSI/WebAPI#2331 to enable temporal annual distribution extending the existing temporal functionality

Each Covariate in a table depending on the FeatureAnalysis support of temporal and temporal annual distribution and analysis execution results availability (activated while configuring an analysis design) will allow to explore temporal details in a separate window to be opened after clicking on a corresponding link

[anthonysena]

Depends on OHDSI/FeatureExtraction#271

[chrisknoll]

The checkboxes are not read-only if the user has read-access. I'll push an update:

[chrisknoll]

As I was fixing the read-only state, I also ran into a comple of other issues:

1. computed() vs. pureComputed

Found isAnnualPrevalenceSupported and isTemporalPrevalenceSupported set up as a computed(). It should be pureComputed() and it's pretty easily fixed.

2. Functional programming confusion

This code:

          this.isAnnualPrevalenceSupported = ko.pureComputed(() => params.design().featureAnalyses().reduce((a, v) => a || v.supportsAnnual, false));
          this.isTemporalPrevalenceSupported = ko.pureComputed(() => params.design().featureAnalyses().reduce((a, v) => a || v.supportsTemporal, false));

I believe the expression that those 2 pure computeds are returning is simply 'is any supportsAnnual == true in the collection?'

The way the reduce() function is applied above, the operation would be described as 'reduce an array of boolean values to a single boolean value where where the subsequent yielded result is true once a true is observed'.

The question is: why express it like that? Isn't the closest approximation to represent the aforementioned logic is:

params.design().featureAnalyses().some((a) => a.supportsAnnual );

The above reads: 'There is some analysis where supportsAnnual is true'. The above may need to be written a bit more complex to handle null cases (or other corner cases, but I don't think we'd need to do that since null and empty values are falsey).

I'd like to understand the justification/rationalization of using a reduce() expression instead of some(). I'm not anti-functional programming, but I am pushing towards simple, clean and readable code. But also code that is robust, so if there's something I'm not understanding about reduce() vs some() when performing this logical operation that makes it more robust, I'd like to understand why.

[chrisknoll]

Looking further at this, the enable/disable behavior based on if there are temporal/annual features included in the design is flawed). Steps to reproduce:

1. Add a set of analyses where some have temporal available, and some do not.
2. Observe checkboxes enabled.
3. Remove the analyses where temporal is available.
4. Observe checkboxes are disabled but the user selection is unchangeable now.

The user request to include the annualized/temporal statistics should be always available as part of the characterization design. If they never selected anything that supports A/T, no harm (we could come up with a message in the future). But disabling a checkbox or (worse) unchecking a checkbox on the user behalf because of selections they made is confusing. The list of features at the top clearly indicates which analyses provide A/T, so adding additional behavior to enable/disable checkboxes based on user selection is unnecessary.

I've reflected this change in latest commit. Please provide any comments you have.

[chrisknoll]

@oleg-odysseus : one confusion is the checkboxes, it's not clear which checkbox shows which data where. What I'm assuming is:

"Include Prevalence Annual Distribution" will make each prevalence item clickable, and clicking on it opens the dialog box with the year column.
"Include Temporal Distribution": Not sure. I have 2 features (Charson Index and Demographics Time In Cohort) in my CC design, but I'm not sure what I should be seeing around those 2 results.

In addition, when 2 cohorts are selected we should see a Standard Difference calculation, but the column is empty:

And apologies for getting this information so slowly, I wanted to run the characterization on a 'real' dataset but on my local workstation, and the execution took over 10 hours. I'm checking to see if it's related to the new design options (is it just my laptop is slow, or if including the new options causes the whole analysis to be slow). Will let you know what I find.

[chrisknoll]

Another bug I found, should be a quick fix but this is something I've seen before:

The error is because the text has a ' in it and breaks the HTML value. I can try to track down where we account for this in other places in the code (like, i seem to recall this being an issue in the tooltip binding when we are puttin a concept name with an apostrophe in it in the tool tip, we had to fix it there too).

In any case, this should be an easy fix so I'm ok to hold up merging this in if we can address it.

@oleg-odysseus @alex-odysseus

[chrisknoll]

Found and fixed it.