[hold] Improve user-facing error message

[abought]

Purpose

Patch proposed by @benralexander . PR created on his behalf by request.

A developer-focused error was being shown to users (likely in this block) Replace that error with more user-friendly text.

Ben provides the following notes about how to reproduce:

If users sometimes specify a genomic range in which no variants exist (definitely easy to do in some of our data sets, and especially if they have 'zoomed in') then the default error message was leaving them confused. The message we've provided instead at least gives them a hint about what they should do next. Clearly a more general approach would provide the calling routine with more options for error handling in this common case.

[pjvandehaar]

Is it possible for this error to occur when variants were returned but they lacked the fields id and pvalue?

[abought]

Peter and I traced through some logic earlier, and I concur. Will talk to Ben about this proposal during tomorrow's technical call.

Although a developer facing error message shouldn't be shown to users, I like that there is a better option for suppressing it upstream in the code.

Long term, I think this exposes a challenge for new users, in that the data sources sometimes embody complex and API-specific assumptions about data sources and required payload structures. (Given that data sources are swappable, we're still doing pretty well for modularity overall!)

We'll see what Ben says, and I can help them trace the difference in environments further if needed.

[benralexander]

That all sounds good to me, and I can implement the strategy that Chris describes. Clearly modifying the core library with an error message that addresses only the symptom our users were seeing was only a work-around, and I'm happy to have a more systematic solution available.

Thanks for the help!

On Tue, Sep 12, 2017 at 10:20 PM, Andy Boughton notifications@github.com wrote:

Peter and I traced through some logic earlier, and I concur. Will talk to Ben about this proposal during tomorrow's technical call.

Although a developer facing error message shouldn't be shown to users, I like that there is a better option for suppressing it upstream in the code.

Long term, I think this exposes a challenge for new users, in that the data sources sometimes embody complex and API-specific assumptions about data sources and required payload structures. (Given that data sources are swappable, we're still doing pretty well for modularity overall!)

We'll see what Ben says, and I can help them trace the difference in environments further if needed.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/statgen/locuszoom/pull/111#issuecomment-329037749, or mute the thread https://github.com/notifications/unsubscribe-auth/AA_6evsywdjiGN25RZV1M9FyTV9ouMgWks5shzwJgaJpZM4PVV2G .

[abought]

Thanks Ben! We'll touch base later today, and can close the PR once we're sure the proposed alternative works for you. I'll also target adding some documentation around the issue uncovered here.

[abought]

Brief follow up: @benralexander , have you had an opportunity to try the alternate proposed fix yet?

Current plan is to merge / test a few things at once this Wednesday; currently this PR serves as a tracking ticket, and may be closed at that time to avoid developing a backlog.

EDIT: As discussed in meeting, Ben indicated that the proposed config change isn't sufficient to fix their issue; I've asked him to post a minimal reproducible test case for followup.

[abought]

Ben has provided a reproducible test case on the portal (a region where we find no available data, without needing to scroll too far). I'll log here for now, and resume debugging as time allows. (hopefully next week)

1. Go to the "metabochip" dataset (which has clusters of data) and select the Locuszoom plot option

2. Zoom in (eg to a narrow band such as 90kb), using shift+scroll if necessary (there may not be zoom buttons). Pan left so that your screen is centered around a region without data; this one worked for me: Region: 8:118105025-118265025

3. The plot should show Ben's custom error message, which modifies LZ source code.

Here is where the error message is implemented in their fork, for comparison: https://github.com/broadinstitute/dig-diabetes-portal/blob/2531871555673a98578a6cbafae244b74fb8e535/web-app/js/lib/locuszoom.app.js#L7801

Below is the exact plot state information extracted from the console:

> JSON.stringify(mpgSoftware.locusZoom.locusZoomPlot['#lz-47'].state, null, 4)

"{
    \"ldrefvar\": \"8:118185025_G/A\",
    \"genes\": {},
    \"genes.genes\": {
        \"highlighted\": [],
        \"selected\": [],
        \"faded\": [],
        \"hidden\": []
    },
    \"chr\": \"8\",
    \"start\": 118081387,
    \"end\": 118171499,
    \"model\": {
        \"covariates\": []
    },
    \"T2D_static_GWAS_DIAGRAM_eu_mdv29GWAS_DIAGRAM_eu_mdv29\": {},
    \"T2D_static_GWAS_DIAGRAM_eu_mdv29GWAS_DIAGRAM_eu_mdv29.significance\": {
        \"highlighted\": [],
        \"selected\": [],
        \"faded\": [],
        \"hidden\": []
    },
    \"T2D_static_GWAS_DIAGRAM_eu_mdv29GWAS_DIAGRAM_eu_mdv29.recombrate\": {
        \"highlighted\": [],
        \"selected\": [],
        \"faded\": [],
        \"hidden\": []
    },
    \"T2D_static_GWAS_DIAGRAM_eu_mdv29GWAS_DIAGRAM_eu_mdv29.associationpvalues\": {
        \"highlighted\": [],
        \"selected\": [],
        \"faded\": [],
        \"hidden\": []
    }
}"

[abought]

One more small trick: rather than scrolling manually, this can be controlled from the developer JS console in the browser once the sample page is loaded (and the plot has been opened). For my own future notes, here are the commands used to reach a region with no GWAS data. (worked as of Jan 11, 2018)

var thePlot = mpgSoftware.locusZoom.locusZoomPlot['#lz-47'];
thePlot.state.start = 118081387;
thePlot.state.end = 118171499;
thePlot.applyState();

[abought]

Per @benralexander meeting: we'd like to make the error message customizable, but we're not 100% sure this specific message is the one we'd want to change.

Based on the test case above, below are sample API payloads for a region with and without data.

I noticed that some of the columns are missing entirely in the latter case- rather than "no data available", this specific error message roughly translates to "cannot reliably parse the API response as expected". Do you think this is worth investigating on your end before we develop a fix?

With data available in region: http://www.type2diabetesgenetics.org/gene/getLocusZoom?chromosome=8&start=118105025&end=118265025&phenotype=T2D&dataset=GWAS_DIAGRAM_eu_mdv29&propertyName=P_VALUE&datatype=static

No data available in region: http://www.type2diabetesgenetics.org/gene/getLocusZoom?chromosome=8&start=118081387&end=118171499&phenotype=T2D&dataset=GWAS_DIAGRAM_eu_mdv29&propertyName=P_VALUE&datatype=static

[abought]

Hi, @benralexander ! Last week, you mentioned that an API bugfix had been shipped. I just tried the reproducible testcase above, and the payload still looks the same as in my previous comment.

Can you clarify whether the API bugfix is in production yet? API URL: http://www.type2diabetesgenetics.org/gene/getLocusZoom?chromosome=8&start=118081387&end=118171499&phenotype=T2D&dataset=GWAS_DIAGRAM_eu_mdv29&propertyName=P_VALUE&datatype=static

Demo page: http://www.type2diabetesgenetics.org/variantInfo/variantInfo/8_118185025_G_A

var thePlot = mpgSoftware.locusZoom.locusZoomPlot['#lz-47'];
thePlot.state.start = 118081387;
thePlot.state.end = 118171499;
thePlot.applyState();

Dataset to select: DIAGRAM GWAS + MetaboChip (static)

Response:

{"lastPage":null,"data":{"refAlleleFreq":[],"analysis":[]},"result_format":"flat","is_error":false,"header":"{}"}

[benralexander]

The API code has been fixed on our dev machines, but the fix is not yet in production. We are hoping to deploy our newest code within the next couple of days.

Thanks for you attention to these topics, Andy!

-Ben

On Tue, Feb 27, 2018 at 6:00 PM, Andy Boughton notifications@github.com wrote:

Hi, @benralexander https://github.com/benralexander ! Last week, you mentioned that an API bugfix had been shipped. I just tried the reproducible testcase above, and the payload still looks the same as in my previous comment.

Can you clarify whether the API bugfix is in production yet? API URL: http://www.type2diabetesgenetics.org/ gene/getLocusZoom?chromosome=8&start=118081387&end= 118171499&phenotype=T2D&dataset=GWAS_DIAGRAM_eu_mdv29& propertyName=P_VALUE&datatype=static

Demo page: http://www.type2diabetesgenetics.org/variantInfo/variantInfo/8_ 118185025_G_A

var thePlot = mpgSoftware.locusZoom.locusZoomPlot['#lz-47'];thePlot.state.start = 118081387;thePlot.state.end = 118171499;thePlot.applyState();

Response:

{"lastPage":null,"data":{"refAlleleFreq":[],"analysis":[]},"result_format":"flat","is_error":false,"header":"{}"}

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/statgen/locuszoom/pull/111#issuecomment-369058312, or mute the thread https://github.com/notifications/unsubscribe-auth/AA_6eoKkaX09TltIStHhpq0azFy3yhXtks5tZIkVgaJpZM4PVV2G .

[abought]

With the API tweaked, a new error message has emerged. This one will require some code changes to fix.

Origin: The association source seems ok with returning no data, but the LD source- which takes in assoc results fetched previously and grafts on the information required for colors- is not: https://github.com/abought/locuszoom/blob/985862f346d28ae4c2ce06bcb1f3e6868e64c25b/assets/js/app/Data.js#L611 https://github.com/abought/locuszoom/blob/985862f346d28ae4c2ce06bcb1f3e6868e64c25b/assets/js/app/Data.js#L689-L692

Since association data returns an empty array of records, there are no fields to merge on, and LD throws an error. A candidate fix would be to skip the merge process if chain.body.length === 0 ; will try this out and vet today.

Target is a release early next week prior to Ben's demo Thursday.

[abought]

An alternate fix has been proposed in #128 (update of a branch shared with Ben previously).

Somewhat ironically, at the moment our own association API endpoint turns out to have a problem with regions where there is no data. (I choose to spin this as "your feedback helps us all to improve")

@benralexander , would you care to try that branch and see if it resolves both the error message and the diagonal line of significance? Same place as before, newer commit. As an added incentive, the new branch also adds an experimental source map for locuszoom.app.min.js, to improve your future debugging experience in production.

The fix is built around the assumption that the LD source is inherently coupled to association data (it is designed to annotate previous data in the chain), and so the LD fetch can be entirely skipped if association results return no data.

If it turns out that you have a means of using the LD source as a standalone, the proposed fix could have unwanted side effects. But it seems reasonable for all the use cases I can think of.

[benralexander]

Excellent. Thanks, Andy! I'll give it a try.

-Ben

On Thu, Apr 12, 2018 at 10:19 PM, Andy Boughton notifications@github.com wrote:

An alternate fix has been proposed in #128 https://github.com/statgen/locuszoom/pull/128 (update of a branch shared with Ben previously).

Somewhat ironically, at the moment our own association API endpoint turns out to have a problem with regions where there is no data. (I choose to spin this as "your feedback helps us all to improve")

@benralexander https://github.com/benralexander , would you care to try that branch and see if it resolves both the error message and the diagonal line of significance? Same place as before, newer commit. As an added incentive, the new branch also adds an experimental source map for locuszoom.app.min.js, to improve your future debugging experience in production.

The fix is built around the assumption that the LD source is inherently coupled to association data (it is designed to annotate previous data in the chain), and so the LD fetch can be entirely skipped if association results return no data.

If it turns out that you have a means of using the LD source as a standalone, the proposed fix could have unwanted side effects. But it seems reasonable for all the use cases I can think of.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/statgen/locuszoom/pull/111#issuecomment-381000921, or mute the thread https://github.com/notifications/unsubscribe-auth/AA_6evVJ-mMxshyq1WddvD-ZYzQVZmLTks5toAs7gaJpZM4PVV2G .

[abought]

Fix has been merged, and closing this ticket ahead of tomorrow's planned 0.7.2 release.