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 medical action association table to UI (plus fixture & enum regen) #937

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

kevinschaper
Copy link
Member

@kevinschaper kevinschaper commented Jan 9, 2025

Adds a medical action table showing the MaXO term, predicate, phenotype and disease. For simplicity sake, the section is labeled as "Medical Action" on both MaXO pages and phenotype pages.

Creating as a draft, because I realized that it needs to show up on disease pages as well, which means that the API request will need to change so that when "entity=" comes into the api request, disease_context_qualifier is searched, and I think we'll also want to make sure we're searching disease_context_qualifier_closure as well (which is hopefully already getting generated!

Closes #919
Closes #935

Copy link

netlify bot commented Jan 9, 2025

Deploy Preview for monarch-app canceled.

Name Link
🔨 Latest commit 9ee736d
🔍 Latest deploy log https://app.netlify.com/sites/monarch-app/deploys/6786f1cf4700fb00085ff197

Copy link

codecov bot commented Jan 9, 2025

Codecov Report

Attention: Patch coverage is 95.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 70.77%. Comparing base (da511ba) to head (b898670).

Files with missing lines Patch % Lines
...rc/monarch_py/implementations/solr/solr_parsers.py 33.33% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #937      +/-   ##
==========================================
- Coverage   71.26%   70.77%   -0.49%     
==========================================
  Files          91       91              
  Lines        3149     3100      -49     
==========================================
- Hits         2244     2194      -50     
- Misses        905      906       +1     

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

self.nlp = spacy.load(str(str(model_archive.parent / model_subdir.name / inner_model_dir.name / model_subdir.name)))
self.nlp = spacy.load(
str(str(model_archive.parent / model_subdir.name / inner_model_dir.name / model_subdir.name))
)

Copy link
Member Author

Choose a reason for hiding this comment

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

Everything in this file is just formatting changes that got picked up

Copy link
Collaborator

Choose a reason for hiding this comment

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

Check, I'm ignoring it then.

@@ -4,6 +4,9 @@ on:
pull_request:
workflow_dispatch:

env:
PYTHONPATH: ${{ github.workspace }}/backend

Copy link
Member Author

Choose a reason for hiding this comment

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

fix for #935

@kevinschaper kevinschaper marked this pull request as ready for review January 9, 2025 21:54
@kevinschaper kevinschaper assigned kevinschaper and unassigned ptgolden Jan 9, 2025
@kevinschaper kevinschaper requested a review from ptgolden January 9, 2025 21:54
@kevinschaper
Copy link
Member Author

@amc-corey-cox @ptgolden The more straightforward part of this is the specific handling for MaXO associations in the UI. The weird part is that by default, we want disease pages to return these MaXO associations in the association-counts part of the entity response, and be able to fetch an association table for MaXO associations where the ID of the page that we're on is a MONDO ID which is in the disease_context_qualifier or disease_context_qualifier_closure field. I handled that by expanding what is matched in the API endpoint for the entity param, since we intend that to return associations involving that entity, and this is the first situation we have where we want the association to show up on a page based on the ID appearing in a qualifier field. I don't know if it's a 100% perfect solution, but I talked myself into the idea that it's a pretty reasonable one.

Copy link
Collaborator

@amc-corey-cox amc-corey-cox left a comment

Choose a reason for hiding this comment

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

I didn't review the big diffs in tests/ or frontend/fixtures but I don't see anything that jumps out as concerning to me. I flagged a couple of things just to make sure you noticed them and that they were done on purpose.
There doesn't appear to be a deploy preview. Would you like me to review the frontend view?

WEB_PAGE = "biolink:WebPage"
ZYGOSITY = "biolink:Zygosity"
MOLECULAR_ENTITY = "biolink:MolecularEntity"
LIFE_STAGE = "biolink:LifeStage"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm going to presume that all of these deletions and additions make sense, although I admit I didn't check to make sure we don't need these.

CHEMICAL_OR_DRUG_OR_TREATMENT_TO_DISEASE_OR_PHENOTYPIC_FEATURE_ASSOCIATION = (
"biolink:ChemicalOrDrugOrTreatmentToDiseaseOrPhenotypicFeatureAssociation"
)
VARIANT_TO_PHENOTYPIC_FEATURE_ASSOCIATION = "biolink:VariantToPhenotypicFeatureAssociation"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Again, I'll presume your reorganization is for improving readability or logical consistency.

):
# This is a special case for disease_context_qualifier, if an association between two other entities
# only occurs within the context of a disease, we can treat it like an incoming association
direction = AssociationDirectionEnum.incoming
Copy link
Collaborator

Choose a reason for hiding this comment

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

Codecov is complaining that 261 and 267 aren't covered by tests. It seems reasonable to ignore this but if it's easy to make a test here it would be nice to cover these edge cases.

self.nlp = spacy.load(str(str(model_archive.parent / model_subdir.name / inner_model_dir.name / model_subdir.name)))
self.nlp = spacy.load(
str(str(model_archive.parent / model_subdir.name / inner_model_dir.name / model_subdir.name))
)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Check, I'm ignoring it then.

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.

Pytest seems to be failing in certain environments Update UI for MAxO
3 participants