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

Add chart drilldown to concept pages #2806

Merged
merged 3 commits into from
Jan 10, 2023

Conversation

anton-abushkevich
Copy link
Contributor

Resolves #2805

@chrisknoll
Copy link
Collaborator

From the UI perspective, let's remove the 'time series' functionality from the concept sets: the way the aggregation is done across concept sets (talking the average of p10 or summing the monthly prevalence) is incorrect math, and an alternative approach to presenting the data could be identified.

The drilldown reports from the concept view is fine and helpful: it saves navigating to data-sources and finding the concept in the right domain. This is helpful and can stay. I'd keep that function for 2.13, and think about how to deal with 'concept-set reporting' as a 2.14 feature.

@TitrS TitrS force-pushed the add_chart_drilldown_to_concept_sets_pages branch from fe7467a to 2359340 Compare January 9, 2023 13:36
@TitrS TitrS requested a review from chrisknoll January 9, 2023 13:42
@TitrS TitrS changed the title Add chart drilldown to concept sets pages Add chart drilldown to concept pages Jan 9, 2023
@TitrS
Copy link
Contributor

TitrS commented Jan 9, 2023

@chrisknoll Hi Chris! I deleted last commits and keep commits for the concept drilldown report. I renamed PR also.

@chrisknoll
Copy link
Collaborator

Thanks. For future reerence, be careful about force-pushing a branch, if you want to go back to a prior commit, just revert to the desired commit which will cause git to make a 'revert commit' that removes the changes. I was lucky to notice you forced pushed the branch because if I tried to merge your latest commits' back to my local branch, it would have been chaos.

@chrisknoll
Copy link
Collaborator

Ok, The Atlas update looks good, but I'd like to also remove the endpionts on the WebAPI side that has those improper calculations that aggregate statistics. Before merging the Atlas side, let's remove the code from webAPI. To do so, please revert commits (don't force push branches) and I'll take a look.

@TitrS
Copy link
Contributor

TitrS commented Jan 10, 2023

Hi @chrisknoll thanks for your comments. If we decide don't add the functionality for concept sets page, you just should reject PR on the WebApi side OHDSI/WebAPI#2178, because on the WebApi side all changes are related to concept sets page.

@chrisknoll chrisknoll merged commit 677e1b8 into master Jan 10, 2023
@delete-merged-branch delete-merged-branch bot deleted the add_chart_drilldown_to_concept_sets_pages branch January 10, 2023 22:07
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.

Add Achilles drilldown reports to Concept / Concept Set pages
3 participants