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

Enable users to filter genes by differential expression metrics (SCP-5090) #1823

Merged
merged 25 commits into from
Jul 17, 2023

Conversation

eweitz
Copy link
Member

@eweitz eweitz commented Jul 13, 2023

This enables users to find genes by significance and size of differential expression.

Previously (#1768), genes could only be sorted by these differential expression metrics. Now, DE genes can also be filtered to exclude results that aren't significant, or have low levels or differential expression, or both. New range slider facets let users filter by those metrics. This satisfies a stakeholder request to enable ranking genes by log2(fold change) after applying a <= 0.05 threshold for adjusted p-value.

Here's how it looks!

Range_filter_slider_facet_for_DE_genes_table__SCP_2023-07-13.mov

In addition to layout shown above, the facets are also somewhat responsive to different viewport widths. Analytics on changes to the range facets are logged to Mixpanel.

Test

Enhanced automated tests verify some range facet functionality. To manually test:

  • Go to "Human milk" DE pilot study
  • Select annotation "cell_type__ontology_label"
  • Click "Differential expression" button
  • Change range slider facets for "log2(fold change)" and "Adj. p-value" to filter for different ranges
  • Confirm DE table changes, either in DE genes shown on page in view or in page count at bottom
  • Click checkbox beside range facet, confirm it makes the facet inactive -- i.e. doesn't apply any filters in that facet
  • If you have author DE populated for the "General_Celltype" annotation:
  • Select annotation "General_Celltype"
  • Confirm range facet "Adj. p-value" does not appear, and that "q-value" does instead
  • Change facets, confirm shown DE genes satisfy them

This satisfies SCP-5090.

@eweitz eweitz requested review from bistline and ehanna4 July 13, 2023 13:59
Copy link
Contributor

@bistline bistline left a comment

Choose a reason for hiding this comment

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

Really great! Looking forward to this being available.

Side note that doesn't warrant action here: in the case were a user selects a comparison in the differential expression picker, and the applied filters result in no matches, should we consider some kind of notification to that effect? Otherwise, it is indistinguishable from being unable to retrieve results from the bucket.

@@ -166,7 +168,7 @@ function searchGenesFromTable(selectedGenes, searchGenes, logProps) {
/** Table of DE data for genes */
function DifferentialExpressionTable({
genesToShow, searchGenes, clusterName, annotation, species, numRows,
bucketId, deFilePath, handleClear, isAuthorDe
bucketId, deFilePath, handleClear, isAuthorDe, facets
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if facets is potentially confusing here - we have plans to add actual search facets to many of our Explore tab plots this quarter, and this might get overloaded.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed, this confusion seems inherent in having two search UIs in view at the same time. The search UI in the DE panel queries a corpus of genes, and the search UI for the plots queries a corpus of cells.

To mitigate potential future confusion, in a489534 I just prefixed a lot these DE search variables with "de", so the cell search variables can be unambiguously named without a prefix.

@eweitz
Copy link
Member Author

eweitz commented Jul 13, 2023

in the case were a user selects a comparison in the differential expression picker, and the applied filters result in no matches, should we consider some kind of notification to that effect?

Yes, that makes sense! For our reference, we just noted we'll do this as part of SCP-5227.

Copy link
Contributor

@ehanna4 ehanna4 left a comment

Choose a reason for hiding this comment

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

Looks good! confirmed works locally

Co-authored-by: ehanna4 <[email protected]>
@eweitz
Copy link
Member Author

eweitz commented Jul 17, 2023

The build failure seems like a false positive, since the cause was a Rails test failure but not Rails code changed.

### FAILURES and ERRORS ###
*** Yarn test failures ***

*** Rails test failures ***

  FireCloudClientTest#test_delete_workspace:
  RuntimeError: Wrapped undumpable exception for: RestClient::InternalServerError: 500 Internal Server Error
Exiting with code: 1

The specific failure line essentially uninformative:

fire_cloud_client_test.rb:148 test_delete_workspace completed (20.843 sec)
E

I saw no test-logs output, I suspect because this branch doesn't yet include #1820. However, the false-positive rationale seems sufficient to merge now.

@eweitz eweitz added the build failure: false positive Build error confirmed as false positive. E.g. upstream service has a problem. label Jul 17, 2023
@eweitz eweitz merged commit a6d973b into development Jul 17, 2023
1 check failed
@eweitz eweitz deleted the ew-de-range-facets branch July 17, 2023 21:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build failure: false positive Build error confirmed as false positive. E.g. upstream service has a problem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants