statgen / locuszoom

A Javascript/d3 embeddable plugin for interactively visualizing statistical genetic data from customizable sources.
https://statgen.github.io/locuszoom/
MIT License
154 stars 29 forks source link

Widget `zoom_region` fails #270

Closed sir4ur0n closed 2 years ago

sir4ur0n commented 2 years ago

Hi!

I am currently fiddling with LocusZoom, and I don't manage to use zoom_region widget. It keeps failing (see error below) and I could not find a single example using such a widget on the examples page to cargo-cult my way :smile:

The Chrome JS error:

Error: <g> attribute transform: Expected number, "translate(NaN,0)".

  | (anonymous) | @ | attr.js:31
  | __WEBPACK_DEFAULT_EXPORT__ | @ | each.js:5
  | __WEBPACK_DEFAULT_EXPORT__ | @ | attr.js:53
  | axis | @ | axis.js:102
  | __WEBPACK_DEFAULT_EXPORT__ | @ | call.js:4
  | renderAxis | @ | panel.js:1411
  | (anonymous) | @ | panel.js:708
  | render | @ | panel.js:687
  | (anonymous) | @ | panel.js:1205
  | Promise.then (async) |   |  
  | reMap | @ | panel.js:1203
  | applyState | @ | plot.js:761
  | (anonymous) | @ | widgets.js:1267
  | (anonymous) | @ | on.js:27

I am using a default LocusZoom toolbar LocusZoom.Layouts.get("toolbar", "region_nav_plot").

It is very likely that I did something wrong rather than a bug in LocusZoom, but without an example, it's a bit hard for me to compare and find the root cause in my code :sweat_smile:

By looking at the LocusZoom code, I saw many uses of translate... I randomly tried e.g., to add origin: {x: 0, y: 0} to my plot state, but no success.

Side note: the z- button is always greyed out/disabled, I'm not sure why either.

abought commented 2 years ago

Thanks for the question. Could you show me your usage/ configuration for comparison? Sometimes errors occur because a required configuration option is missing, and I try to improve error messages/ validation based on bug reports.

I'm going to pull down the code and verify this on my end to get you a better example- we de-emphasized this widget in favor of shift-mousewheel "scroll to zoom" behavior. But we've gotten some feedback that this is hard to discover for new users, and I've been thinking it's worth adding this control back to the default plot.

sir4ur0n commented 2 years ago

I'll do my best to redact while still giving the bits that might be of interest to you.

Please bear with me as I'm not an experienced JS developer at all :sweat_smile: (and by the way I'll gladly take any criticism to improve):

import LocusZoom from "locuszoom";
import "locuszoom/dist/locuszoom.css";

const element = document.createElement("div");
element.id = "lz-plot";

document.body.appendChild(element);

const apiBase = "https://portaldev.sph.umich.edu/api/v1/";
const AssociationLZ = LocusZoom.Adapters.get("AssociationLZ");

class CustomAssociationLZ extends AssociationLZ {
  _getURL(_request_options) {
    return this._url;
  }
  // Override this function to call as POST and pass our custom arguments
  _performRequest(options) {
    const url = this._getURL(options);
    // Many resources will modify the URL to add query or segment parameters. Base method provides option validation.
    //  (not validating in constructor allows URL adapter to be used as more generic parent class)
    if (!this._url) {
      throw new Error(
        'Web based resources must specify a resource URL as option "url"'
      );
    }
    const customQuery = { custom: "query relying on options.chr, options.start, options.end" };
    const requestBody = {
      method: "POST",
      headers: {
        "Content-Type": "application/json",
      },
      body: JSON.stringify(customQuery), // body data type must match "Content-Type" header
    };
    return fetch(url, requestBody).then((response) => {
      if (!response.ok) {
        throw new Error(response.statusText);
      }
      const result = response.text();
      return result;
    });
  }

  /**
   * Customize because our API returns a row structure
   */
  _normalizeResponse(response_text, options) {
    const data = JSON.parse(response_text);
    const keys = data.columns;
    const records = data.rows.map((row) => {
      const record = {};
      keys.map((key, index) => (record[key] = row[index]));
      // Extra work to perfectly fit the UMich data format
      record["analysis"] = record["study_id"];
      record["beta"] = record["effect_size"];
      record["chromosome"] = options.chr;
      record["log_pvalue"] = record["neglogp"];
      record["position"] = record["pos"];
      record["ref_allele"] = record["ref"];
      record["ref_allele_freq"] = null;
      record["score_test_stat"] = null;
      record["se"] = record["stderr"];
      record["variant"] = "".concat(
        options.chr,
        ":",
        record["pos"],
        "_",
        record["ref"],
        "/",
        record["alt"]
      );
      return record;
    });
    return records;
  }

  _annotateRecords(records, options) {
    return super._annotateRecords(records, options);
  }
}

// Register our custom adapter to the global registry
LocusZoom.Adapters.add("CustomAssociationLZ", CustomAssociationLZ);

const data_sources = new LocusZoom.DataSources()
  .add("assoc", [
    "CustomAssociationLZ",
    {
      url: "http://my_custom_url/my_custom_endpoint",
    },
  ])
  // I know all the builds are inconsistent with state.build, don't worry about it :D
  .add("ld", [
    "LDServer",
    {
      url: "https://portaldev.sph.umich.edu/ld/",
      source: "1000G",
      population: "ALL",
      build: "GRCh37",
    },
  ])
  .add("recomb", [
    "RecombLZ",
    {
      url: apiBase + "annotation/recomb/results/",
      build: "GRCh37",
    },
  ])
  .add("gene", [
    "GeneLZ",
    {
      url: apiBase + "annotation/genes/",
      build: "GRCh37",
    },
  ])
  .add("constraint", [
    "GeneConstraintLZ",
    {
      url: "https://gnomad.broadinstitute.org/api/",
      build: "GRCh37",
    },
  ]);

// Unlike regular widgets, `set_state` widgets can only be plot-wise, not panel-wise
const changeChromosomeWidget = {
  type: "set_state",
  button_html: "Chromosome: ",
  button_title: "Select the chromosome",
  show_selected: true,
  state_field: "chr",
  options: [
    { display_name: "1", value: "1" },
    { display_name: "2", value: "2" },
    { display_name: "3", value: "3" },
    { display_name: "4", value: "4" },
    { display_name: "5", value: "5" },
    { display_name: "6", value: "6" },
    { display_name: "7", value: "7" },
    { display_name: "8", value: "8" },
    { display_name: "9", value: "9" },
    { display_name: "10", value: "10" },
    { display_name: "11", value: "11" },
    { display_name: "12", value: "12" },
    { display_name: "13", value: "13" },
    { display_name: "14", value: "14" },
    { display_name: "15", value: "15" },
    { display_name: "16", value: "16" },
    { display_name: "17", value: "17" },
    { display_name: "18", value: "18" },
    { display_name: "19", value: "19" },
    { display_name: "20", value: "20" },
    { display_name: "21", value: "21" },
    { display_name: "22", value: "22" },
    { display_name: "X", value: "X" },
    { display_name: "Y", value: "Y" },
    { display_name: "MT", value: "MT" },
  ],
};

const plotToolbar = LocusZoom.Layouts.get("toolbar", "standard_plot");
plotToolbar.widgets.push(changeChromosomeWidget);

const associationPanel = LocusZoom.Layouts.get("panel", "association", {});

// Add region navigation buttons to the panel
associationPanel.toolbar.widgets.push(
  ...LocusZoom.Layouts.get("toolbar", "region_nav_plot").widgets
);

const layout = {
  width: 800,
  panels: [associationPanel],
  toolbar: plotToolbar,
  state: {
    genome_build: "GRCh38",
    chr: "2",
    start: 500_000,
    end: 600_000,
  },
};

const plot = LocusZoom.populate("#lz-plot", data_sources, layout);

But we've gotten some feedback that this is hard to discover for new users, and I've been thinking it's worth adding this control back to the default plot.

Might I suggest in addition, a way to select the start/end by typing the numbers in a field? This is sometimes useful and faster than scrolling/trying to fit the wished position window. I think this can already be achieved with a filter_field but I have not yet explored this widget and plugging it. Based on the coaccessibility example I suspect it can work :crossed_fingers:

abought commented 2 years ago

I've confirmed that the widget should be working normally in theory.

In practice, we often use the zoom widget as a group of buttons with common move/zoom actions at once. (see image)

Screen Shot 2022-05-19 at 11 58 05 AM

If you're comfortable with that set of buttons all together, you can activate the entire bundle of options like so:

layout.toolbar = LocusZoom.Layouts.get("toolbar", "region_nav_plot");

// Generate the LocusZoom plot, including customizations made to the layout.
window.plot = LocusZoom.populate("#lz-plot", data_sources, layout);

For the details of how each individual "shift" and "zoom" button are defined, see where that "group of widgets" toolbar is defined: https://github.com/statgen/locuszoom/blob/a5587ef0146b95fc1277f355a58ac90c2acabd7a/esm/layouts/index.js#L683-L728

Typically, the "zoom out" (z-) button will be disabled if the region in view is >= plot_layout.max_region_scale . This is a safeguard so that people don't accidentally zoom out to genome-wide scale all at once; few web servers would support a query that large!

sir4ur0n commented 2 years ago

Yes, this is exactly what I use, cf my code snippet above:

const associationPanel = LocusZoom.Layouts.get("panel", "association", {});

// Add region navigation buttons to the panel
associationPanel.toolbar.widgets.push(
  ...LocusZoom.Layouts.get("toolbar", "region_nav_plot").widgets
);

The button correctly displays. The problem happens when I click on z+

abought commented 2 years ago

Might I suggest in addition, a way to select the start/end by typing the numbers in a field? This is sometimes useful and faster than scrolling/trying to fit the wished position window.

Thanks for the suggestion! I don't think you could do this via filter field, actually- that operates on one aspect of data rather than controlling the entire plot.

It's worth noting that you can write custom controls that allow external HTML to modify the plot- you aren't limited to the things we provide by default, and the search box doesn't even need to be part of the LocusZoom plot at all.

You can see a simple example of this in the "top hits" HTML links in our demos, but you could just as easily decide the coordinates via a form <input> field. The key section is that once you have the desired coordinates, you do this:

plot.applyState({ chr: chr, start: start, end: end, ldrefvar: "" });

In practice, we find that many web developers want to control how people search for the region of interest. (like writing a custom search box for rsID, gene name... etc) That's the only reason we haven't added a built-in free text button; most people like to do something a little more custom.

It's hard to predict what sort of custom option someone might want. There's some partial documentation on all the various ways that a plot can communicate with other figures or tables on your web page; probably a lot more arcane details than you need to know for this task. :) https://statgen.github.io/locuszoom/docs/guides/interactivity.html#using-external-commands-to-update-the-plot

abought commented 2 years ago

Incidentally, looking at the code snippet, I noticed that you were doing this:

// Add region navigation buttons to the panel
associationPanel.toolbar.widgets.push(
  ...LocusZoom.Layouts.get("toolbar", "region_nav_plot").widgets
);

The zoom widget acts on all panels, not just one. a) It probably makes sense to add it to plotToolbar rather than associationPanel.toolbar (usually we try to warn if a widget would break if used in the wrong place; sometimes errors slip through!) b) Region nav plot has all the widgets of the default plot, AND some extra. As written, you might get two copies of some buttons because the code says "list of toolbar widgets = (defaults) + (defaults + extra).

And no worries about being new to javascript; from the looks of it, your instincts are good! Calling it a critique implies something wrong with the developer. This is feedback to a colleague; there is no shame in sharing code while trying something new.

sir4ur0n commented 2 years ago

Thank you for all the information :bow:

To clarify your comment https://github.com/statgen/locuszoom/issues/270#issuecomment-1131910995 : did you try zooming in and it works? Because the button displays fine for me, but I still get the error when clicking it, and I don't know how to solve this


Unrelated: I started fiddling with a custom field + button to replot, but plot.applyState({...}) only updates the state, it does not refetch data :thinking:

  1. In CustomAssociationLZ._performRequest I also use a custom state value, say, options.neglogp_threshold
  2. I have created an input field for this value, and a button to plot.applyState when clicked:
    
    const plot = LocusZoom.populate("#lz-plot", data_sources, layout);

// Add custom buttons and fields const neglogpThresholdLabel = document.createElement("label"); neglogpThresholdLabel.setAttribute("for", "neglogpThreshold"); neglogpThresholdLabel.innerText = "-log10(p) threshold: "; document.body.appendChild(neglogpThresholdLabel);

const neglogpThreshold = document.createElement("input"); neglogpThreshold.type = "number"; neglogpThreshold.name = "neglogpThreshold"; neglogpThreshold.id = "neglogpThreshold"; neglogpThreshold.value = 7.301; document.body.appendChild(neglogpThreshold);

const submit = document.createElement("button"); submit.type = "button"; submit.innerText = "Plot"; submit.onclick = function () { plot.applyState({ neglogp_threshold: neglogpThreshold.value, }); }; document.body.appendChild(submit);



When I click that button, the plot state is updated, but the plot is not redrawn (technically, I guess, the data is not fetched again?).
I added a `console.log` into `CustomAssociationLZ._performRequest` so I know it's not fetched again (but if I force the plot redrawing, e.g. by shifting the region, then it correctly picks the new state value).

I read through https://statgen.github.io/locuszoom/docs/guides/interactivity.html#using-external-commands-to-update-the-plot and my original understanding was that it would refetch data then redraw, but I suspect it only redraws...

I also re-read https://statgen.github.io/locuszoom/docs/guides/data_retrieval.html but I don't quite get how I'm supposed to "link" a change of state (via `plot.applyState`) to a data layer re-fetch.

I've read about `subscribeToData` but I feel like it's the opposite of my need: it allows non-LZ elements to be alerted when something changes within LZ, right?
abought commented 2 years ago

Thanks for the notes!

I believe that the problem with not fetching new data is due to the caching mechanism.

  1. To make user interaction snappier, we only fetch new data from the remote server when the "cache key" changes. (calling plot.applyState is equivalent to "get data from cache or server, then re-render")
    • Sometimes you want to perform custom data transformations after the data is returned from the server, but before re-render. (like neg_log_p_threshold) The annotateRecords method is a good place for such logic.
    • performRequest only gets called when cache can't be used. Putting too much logic just in performRequest may explain the odd behavior you are seeing. All those other methods / steps are a way for users to get the benefit of caching, while still giving full control of how their data looks. The exact arrangement of data retrieval steps is here, though I hope most people don't need quite that much detail!
  2. The default cache key for region plots is chr_start_end. Anything that affects the request made to the server should be part of the cache key too. Otherwise, the default cache key of chr_start_end will be used to decide whether to call performRequest and talk to the remote server (example of extra cache key fields)
    • If you are only filtering / modifying data client side (like filtering on a user-entered neglogp_threshold), then you don't need to change the cache key. Only things that modify the request to the web server need to be there.
  3. Depending on how much "pseudocode" is in your example, it's possible that you might have accidentally further hard-coded a specific request no matter what? const customQuery = { custom: "query relying on options.chr, options.start, options.end" };

Lastly: yup, subscribeToData does not control the data request process at all. It's useful if you want to draw an external HTML widget, like a table, that shows the same data as in the plot. (example: the "export" tab in my.locuszoom.org)

sir4ur0n commented 2 years ago

I finally had time to work back on LocusZoom, and your point about _getCacheKey was spot on :ok_hand: Thank you for the help! As soon as I added the extra parameter to the cache key it worked perfectly.

About your other points:

I realize this issue has been drifting from an original problem about zoom_region into a chat about many questions and problems, I'll try to be more disciplined in creating several issues for several unrelated topics in the future :bow:

That being said, the region zoom still breaks for me :smile: I'll try to take some time to prepare a minimal example project (without my private code) to either show the bug, or investigate what I did wrong by dichotomy in case the minimal example works.

sir4ur0n commented 2 years ago

Sorry for the spam, but actually reproducing the problem was fast :sweat_smile:

You can see it in https://github.com/sir4ur0n/locuszoom/blob/bug270regionZoomFails/examples/multiple_phenotypes_layered.html#L96

Basically I changed a single line, the layout toolbar, to LocusZoom.Layouts.get("toolbar", "region_nav_plot").

From the plot (well displayed), clicking on the z+ button fails. image image

abought commented 2 years ago

Aha!

It looks like the problem was specific to the multiple phenotypes demo- I previously tested the widget on our other plots. This allowed me to localize the root cause. There is an immediate fix you can make, and a future release will validate to catch this error automatically.

Basically: the problem was any zoom out operation, not just via the buttons. It was calculating an infinite zoom scale due to a missing value for max_region_scale property. Below, I've shown how your demo should look (note the two new "*_region_scale" properties in plot.layout).

A while back we reworked the demo for clarity, and maybe simplified one item too many. It's probably my fault- I sort of find layered plots hard to read, and it looks like I was slightly less exhaustive on re-testing "multiple layered plots" stuff (I prefer separate stacked tracks, myself)

    layout = {
        width: 800,
        responsive_resize: true,
        max_region_scale: 1e6,
        min_region_scale: 20000,
        panels: [
            LocusZoom.Layouts.get("panel", "association", association_panel_mods),
            LocusZoom.Layouts.get("panel", "genes", { namespace: { "gene": "gene" } })
        ],
        toolbar: LocusZoom.Layouts.get("toolbar","region_nav_plot")
    };
sir4ur0n commented 2 years ago

Perfect, adding the [min|max]_region_scale properties fixes the issue, thank you! :bow:

I don't mind too much keeping these 2 properties in my code, so as far as I'm concerned the issue can be closed, but if you'd prefer keep it open until the root cause is fixed and published in a new version, suit yourself :smile: I'll let you do the call