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

Temporal annual distribution functionality and temporal FeatureExtraction capability enablement #2950

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

alex-odysseus
Copy link
Contributor

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 anthonysena self-assigned this Dec 17, 2024
@anthonysena
Copy link
Collaborator

Depends on OHDSI/FeatureExtraction#271

@chrisknoll
Copy link
Collaborator

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

image

@chrisknoll
Copy link
Collaborator

chrisknoll commented Mar 27, 2025

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.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants