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

Merging intervals tracks doesn't properly resize plot #216

Closed kennethbruskiewicz closed 3 years ago

kennethbruskiewicz commented 3 years ago

When splitting a track, the plot must resize to fit all of the stacked annotations. When merging the track back together, the plot doesn't go back to a size that compactly fits the visible data.

Illustration: merge_track_problem

Stray thought: does the proportional_height of the annotations panel get recalculated properly when it grows and shrinks?

kennethbruskiewicz commented 3 years ago

Configurations at time of writing:

Plot configuration

{
            responsive_resize: false,
            state: {
                chr: this.chr,
                start: this.start,
                end: this.end,
            },
        }

Associations Panel

{
            ...BASE_PANEL_OPTIONS,
            proportional_height: 0,
            min_height: 240,
            id: this.panel_id,
            y_index: 0,
            axes: {
                y1: {
                    label: '-log10(p)',
                }
            },
            data_layers: [
                // this works
                LocusZoom.Layouts.merge(
                    {
                        y_axis: {
                            axis: 1,
                            field: `{{namespace[${this.datasource_type}]}}log_pvalue`,
                            upper_buffer: 0.10,
                        },
                        fields: [
                            `{{namespace[${this.datasource_type}]}}pValue`,  // adding this piece of data irrelevant to the graphic will help us filter later
                            `{{namespace[${this.datasource_type}]}}consequence`,  // adding this piece of data irrelevant to the graphic will help us filter later
                            `{{namespace[${this.datasource_type}]}}nearest`,  // adding this piece of data irrelevant to the graphic will help us filter later
                            // we need to call out the fields directly since merge algorithm doesn't combine arrays
                            `{{namespace[${this.datasource_type}]}}beta`,
                            ...LocusZoom.Layouts.get('data_layer', 'association_pvalues', { unnamespaced: true }).fields,
                        ],
                    },
                    LocusZoom.Layouts.get('data_layer', 'association_pvalues', { unnamespaced: true }),
                ),
                LocusZoom.Layouts.get('data_layer', 'recomb_rate', { unnamespaced: true }),
                LocusZoom.Layouts.get('data_layer', 'significance', { unnamespaced: true })
            ]
        }

Annotations Panel

{
            height: 120,
            min_height: 120,
            y_index: 1,
            title: {
                text: `${annotation} ${method ? method : ''}`
            },
            data_layers: [
                LocusZoom.Layouts.merge(
                    {
                        fields: [
                            `{{namespace[${this.datasource_type}]}}pValue`,
                            `{{namespace[${this.datasource_type}]}}fold`,
                            ...LocusZoom.Layouts.get('data_layer', 'intervals', { unnamespaced: true }).fields
                        ]
                    },
                    LocusZoom.Layouts.get('data_layer', 'intervals', { unnamespaced: true }),
                ),
            ]
        }

BASE_PANEL_OPTIONS at time of writing

{
    height: 240,
    min_height: 240,
}
abought commented 3 years ago

Apologies for the delayed response- I'd been trying to get the tutorial done first, but will spend some time with this tomorrow.

Quick questions:

  1. Is the use of merge required? Typically, the third argument to LocusZoom.Layouts.get() is called overridesand is literally merged with the parent Not broken, just syntactically noisier than needed.
  2. I notice that the plot layout is incomplete. The most common reason I see heights thrash this way is when the plot asks for a certain height, but the initial panels don't add up to that height. Important pieces of information that would be helpful include a) if your plot layout specifies its own height/width/min dimensions, or b) is inheriting from another layout that does.
kennethbruskiewicz commented 3 years ago

The plot does not inherit from any layout nor does it specify its own height.

As a test, I have added height and min_height as properties to the plot config given in my previous post, with various values (including absurd ones like height: 10 and min_height: 0, as well as height and min_height as 345). They don't appear to affect the problem.

abought commented 3 years ago

The plot does not inherit from any layout nor does it specify its own height.

Thanks. I was able to introspect the live code running on the portal, and determine the following configuration parameters for the plot (after first render, no tracks added):

> plot = $0.__data__.getPlot();
> plot.layout.height
360

($0 is a reference to currently selected element- eg I clicked "inspect" on a scatter plot datapoint, and then abused the d3 behavior of binding the element data to a property called __data__)

I've been exploring a fix that involves completely removing the concept of proportional_height and simplifying the logic. This will be a much larger haul (due to backwards compatible testing on 6+ different sites that use LocusZoom), but I think it will be the best fix long term. The current height logic is one of my least favorite tangles at present, and this won't be a quick task, but phase 1 (removing responsive height resizing) is done as of v0.12 and unlocks the potential to remove proportional heights entirely.

No immediate estimate on delivery time. In our last meeting, intervals tracks were raised as the higher priority and that is if anything a larger rewrite. I'll use this ticket to update you on progress with heights as ready.

kennethbruskiewicz commented 3 years ago

Thanks a bunch Andy.

abought commented 3 years ago

Should be fixed in #225, based on initial testing across sites. I'll prepare a 0.13.0-beta4 release in the next day or two.