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

refact!: discovery module + revised overview endpoints #496

Merged
merged 26 commits into from
Apr 12, 2024

Conversation

davidlougheed
Copy link
Member

@davidlougheed davidlougheed commented Mar 27, 2024

BREAKING CHANGE: new formats for various overview endpoints in order to share more code.

beacon: bento-platform/bento_beacon#79

web: bento-platform/bento_web#392

@davidlougheed davidlougheed changed the title [WIP] refact: discovery module [WIP] refact!: discovery module + revised overview endpoints Mar 27, 2024
@davidlougheed davidlougheed changed the title [WIP] refact!: discovery module + revised overview endpoints refact!: discovery module + revised overview endpoints Mar 27, 2024
Copy link

codecov bot commented Apr 5, 2024

Codecov Report

Attention: Patch coverage is 92.04340% with 44 lines in your changes are missing coverage. Please review.

Project coverage is 88.08%. Comparing base (a1a7305) to head (3a0f715).
Report is 21 commits behind head on develop.

Files Patch % Lines
chord_metadata_service/phenopackets/utils.py 51.16% 19 Missing and 2 partials ⚠️
chord_metadata_service/discovery/fields.py 90.97% 6 Missing and 7 partials ⚠️
chord_metadata_service/discovery/fields_utils.py 94.02% 2 Missing and 2 partials ⚠️
chord_metadata_service/discovery/stats.py 90.47% 2 Missing and 2 partials ⚠️
chord_metadata_service/discovery/api_views.py 97.01% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #496      +/-   ##
===========================================
+ Coverage    87.70%   88.08%   +0.37%     
===========================================
  Files          116      127      +11     
  Lines         4467     4566      +99     
  Branches       672      671       -1     
===========================================
+ Hits          3918     4022     +104     
+ Misses         393      390       -3     
+ Partials       156      154       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@v-rocheleau v-rocheleau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First pass, nice asyncio usage for summaries

chord_metadata_service/discovery/api_views.py Show resolved Hide resolved
chord_metadata_service/discovery/stats.py Outdated Show resolved Hide resolved
chord_metadata_service/phenopackets/summaries.py Outdated Show resolved Hide resolved
@davidlougheed
Copy link
Member Author

still working on tests for this, although project coverage may be lower now due to testing of branching.

@davidlougheed davidlougheed marked this pull request as ready for review April 9, 2024 19:48
return stats


async def get_field_options(field_props: DiscoveryFieldProps, low_counts_censored: bool) -> list[Any]:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: make low_counts_censored default to True as a safeguard

chord_metadata_service/discovery/api_views.py Outdated Show resolved Hide resolved
chord_metadata_service/discovery/fields.py Outdated Show resolved Hide resolved
chord_metadata_service/discovery/types.py Outdated Show resolved Hide resolved
chord_metadata_service/patients/api_views.py Outdated Show resolved Hide resolved
chord_metadata_service/patients/api_views.py Outdated Show resolved Hide resolved
chord_metadata_service/phenopackets/utils.py Outdated Show resolved Hide resolved
chord_metadata_service/phenopackets/utils.py Outdated Show resolved Hide resolved
Copy link
Contributor

@v-rocheleau v-rocheleau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Member

@gsfk gsfk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good... I was thrown off at first by the new nesting experiment.data_type_specific.experiments but it seems to make sense. Two issues / questions:

  • katsu has an endpoint api/search_overview that is used both by beacon and by bento_web: it takes a list of individual ids as input, and returns (uncensored) overview statistics for them. The format is the same as the old front page overview with some outer nesting removed. Now overview and search_overview are out of sync. They don't particularly need to be in sync, but it seems inelegant somehow if they're not. In the long term I wonder about the usefulness of a hardcoded overview fomat as opposed to the configurable public one.

  • Is public overview always censored with count_threshold, or are there permissions that can remove the threshold?

@davidlougheed
Copy link
Member Author

regarding point 2, permissions haven't been done yet but will be done in the future

@davidlougheed davidlougheed requested a review from gsfk April 12, 2024 19:22
@gsfk
Copy link
Member

gsfk commented Apr 12, 2024

One oddity about overview censorship (including current master) is it will wipe out entire categories if they are empty. This is fine for pie charts but not necessarily ideal for bar charts (where you may want to show categories with value zero.... think of the age distribution chart that becomes confusing if you remove bars from the middle of the chart if they happen to be empty)

... I guess the same point can apply to sparsely populated categories even when uncensored.

@davidlougheed
Copy link
Member Author

Yeah that's not ideal handling - although if it's in master I won't bother with it here for now

@davidlougheed davidlougheed merged commit fa5ef34 into develop Apr 12, 2024
7 checks passed
@davidlougheed davidlougheed deleted the refact/discovery-and-overviews branch April 12, 2024 20:35
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.

3 participants