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

Specify join for envo term when faceting omics_processing #1116

Merged
merged 3 commits into from
Jan 19, 2024

Conversation

naglepuff
Copy link
Collaborator

@naglepuff naglepuff commented Jan 18, 2024

Fix #1115

This PR changes how subclasses of BiosampleFilter join to the omics_procesing table.

Previously, they didn't specify at all, so when a filter on an envo term was applied to a search on omics_processing records, there was a Cartesian product and no filtering was done.

Now, those subclasses that represent <EnvoTerm>Filters use the join_envo function of the BaseFilter class to perform the join and correctly filter omics processing results.

When faceting omics_processing records on one of the envo terms,
be sure to correctly join with envo terms, instead of using a cartesian
product.
@naglepuff naglepuff marked this pull request as ready for review January 18, 2024 22:41
Copy link
Collaborator

@jeffbaumes jeffbaumes left a comment

Choose a reason for hiding this comment

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

This fix makes a lot of sense to me and your analysis on the parent issue confirms it's working as expected. One thing unclear to me is whether this worked before #991 or if the omics bar chart never worked properly when querying by envo terms, but it is probably not necessary for find out.

I have one consistency-based suggested change.

@naglepuff naglepuff requested a review from jeffbaumes January 19, 2024 15:58
@naglepuff naglepuff force-pushed the 1115-envo-omics-filters branch from db5f617 to 15ebb01 Compare January 19, 2024 15:59
Copy link
Collaborator

@jeffbaumes jeffbaumes left a comment

Choose a reason for hiding this comment

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

Nice, now a one-line change (ignoring formatting), join_omics_processing logic looks exactly parallel to the logic in join_biosample and join_study. Perfect IMO!

@naglepuff naglepuff merged commit 5b310ec into main Jan 19, 2024
2 checks passed
@naglepuff naglepuff deleted the 1115-envo-omics-filters branch January 19, 2024 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Omics type bucketed facet bugged
2 participants