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

Collision between namespace and panel type labels makes `renameFields` prevent panels from being added to LocusZoom? #249

Closed kennethbruskiewicz closed 3 years ago

kennethbruskiewicz commented 3 years ago

The LocusZoom.Layouts.renameFields method recursively scans a data-structure for a string, and replaces matching strings with another given string. This function is useful for modifying namespace prefixes, specifically those values in namespace and fields, as well as scale-functions/toolbar elements that call out for data from within that namespace.

However, the function doesn't appear to discriminate between the properties whose values it rewrites. This is a problem when the namespace prefix has the same literal value as another property. In this case the following error occurs:

Error: Item not found: <replacement string>
    at ClassRegistry.get (base.js:27)
    at ClassRegistry.create (base.js:87)
    at Panel.addDataLayer (panel.js:466)
    at panel.js:873
    at Array.forEach (<anonymous>)
    at Panel.initializeLayout (panel.js:872)
    at new Panel (panel.js:291)
    at Plot.addPanel (plot.js:464)

An example of a layout where this is a problem is the Intervals Annotation Track. For this layout, "intervals" is the name of the main prefix, the type and the tag.

The cases are as follows:

FAILS: The result of modifying an intervals layout with LocusZoom.Layouts.renameFields

{
    // ...
     "data_layers": [
                {
                    "namespace": {
                        "intervals": "intervals__1623361886864_13"
                    },
                    "id": "intervals__1623361886864_13",
                    "type": "intervals__1623361886864_13",
                    "tag": "intervals__1623361886864_13",
                    "fields": [
                        "intervals__1623361886864_13:start",
                        "intervals__1623361886864_13:end",
                        "intervals__1623361886864_13:state_id",
                        "intervals__1623361886864_13:state_name",
                        "intervals__1623361886864_13:itemRgb"
                    ],
                    "id_field": "intervals__1623361886864_13:start",
                    "start_field": "intervals__1623361886864_13:start",
                    "end_field": "intervals__1623361886864_13:end",
                    "track_split_field": "intervals__1623361886864_13:state_name",
                    "track_label_field": "intervals__1623361886864_13:state_name",
                    "split_tracks": false,
                    "always_hide_legend": true,
                    "color": [
                        {
                            "field": "intervals__1623361886864_13:itemRgb",
                            "scale_function": "to_rgb"
                        },
                        {
                            "field": "intervals__1623361886864_13:state_name",
                            "scale_function": "categorical_bin",
                            "parameters": {
                                "categories": [],
                                "values": [],
                                "null_value": "#B8B8B8"
                            }
                        }
                    ],
                    "legend": [],
                    "behaviors": {
                        "onmouseover": [
                            {
                                "action": "set",
                                "status": "highlighted"
                            }
                        ],
                        "onmouseout": [
                            {
                                "action": "unset",
                                "status": "highlighted"
                            }
                        ],
                        "onclick": [
                            {
                                "action": "toggle",
                                "status": "selected",
                                "exclusive": true
                            }
                        ],
                        "onshiftclick": [
                            {
                                "action": "toggle",
                                "status": "selected"
                            }
                        ]
                    },
                    "tooltip": {
                        "namespace": {
                            "intervals": "intervals__1623361886864_13"
                        },
                        "closable": false,
                        "show": {
                            "or": [
                                "highlighted",
                                "selected"
                            ]
                        },
                        "hide": {
                            "and": [
                                "unhighlighted",
                                "unselected"
                            ]
                        },
                        "html": "{{intervals__1623361886864_13:state_name|htmlescape}}<br>{{intervals__1623361886864_13:start|htmlescape}}-{{intervals__1623361886864_13:end|htmlescape}}"
                    }
                }
            ]
}

WORKS: Editing the namespace prefixes by hand.

{
    // ...
   "data_layers": [
        {
            "namespace": {
                "intervals": "intervals_1623361999074_10_src"
            },
            "fields": [
                "intervals_1623361999074_10_src:pValue",
                "intervals_1623361999074_10_src:fold",
                "intervals_1623361999074_10_src:start",
                "intervals_1623361999074_10_src:end",
                "intervals_1623361999074_10_src:state_id",
                "intervals_1623361999074_10_src:state_name",
                "intervals_1623361999074_10_src:itemRgb"
            ],
            "id": "intervals",
            "type": "intervals",
            "tag": "intervals",
            "id_field": "intervals_1623361999074_10_src:start",
            "start_field": "intervals_1623361999074_10_src:start",
            "end_field": "intervals_1623361999074_10_src:end",
            "track_split_field": "intervals_1623361999074_10_src:state_name",
            "track_label_field": "intervals_1623361999074_10_src:state_name",
            "split_tracks": false,
            "always_hide_legend": true,
            "color": [
                {
                    "field": "intervals_1623361999074_10_src:itemRgb",
                    "scale_function": "to_rgb"
                },
                {
                    "field": "intervals_1623361999074_10_src:state_name",
                    "scale_function": "categorical_bin",
                    "parameters": {
                        "categories": [],
                        "values": [],
                        "null_value": "#B8B8B8"
                    }
                }
            ],
            "legend": [],
            "behaviors": {
                "onmouseover": [
                    {
                        "action": "set",
                        "status": "highlighted"
                    }
                ],
                "onmouseout": [
                    {
                        "action": "unset",
                        "status": "highlighted"
                    }
                ],
                "onclick": [
                    {
                        "action": "toggle",
                        "status": "selected",
                        "exclusive": true
                    }
                ],
                "onshiftclick": [
                    {
                        "action": "toggle",
                        "status": "selected"
                    }
                ]
            },
            "tooltip": {
                "namespace": {
                    "intervals": "intervals"
                },
                "closable": false,
                "show": {
                    "or": [
                        "highlighted",
                        "selected"
                    ]
                },
                "hide": {
                    "and": [
                        "unhighlighted",
                        "unselected"
                    ]
                },
                "html": "{{intervals_1623361999074_10_src:state_name|htmlescape}}<br>{{intervals_1623361999074_10_src:start|htmlescape}}-{{intervals_1623361999074_10_src:end|htmlescape}}"
            }
        }
    ]
}

The main diff between the two cases are tags, type, and id retaining their original values. The rewriting of type is likely the culprit, being the information used in LocusZoom.Panel.addDataLayer to find the right base component to instantiate.

The goal would be to find a way to restrict LocusZoom.Layouts.renameFields into only modifying the strings where it matters.

abought commented 3 years ago

After further slack discussion, it appears that the root cause of this bug is that Kenneth was attempting to rename a fragment of a string (intervals) rather than an actual field. This is a definite potential issue, but because almost all fields in LZ are namespaced (like "{{namespace[intervals]}}start"), it is extremely unlikely to happen in practice. renameFields is intended to be used for fields, not fragments.

To apply a different namespace to a field, renameFields is not the best choice. Instead, the layouts registry provides a built-in way of applying namespaces: a third "overrides" parameter when requesting the layout.

Let me know how the documentation can be more clear on this point- I think the portal has sidestepped the namespace and code-reuse mechanisms of LZ in the past, and if we can consolidate on a consistent approach, that would be helpful rather than continuing to support a wide range of weird workarounds.

For example, consider the abstract (reusable) form of a layout as below:

> LocusZoom.Layouts.get('data_layer', 'intervals', { unnamespaced: true })
{
  ...omitted,
  id_field: "{{namespace[intervals]}}start"
}

We can instead request the data-specified form of a layout using namespaces: "when the layout asks for intervals, fill in the name of a specific data source instead". (note the third argument, "overrides", for when we want to customize or override one thing). When we request the concrete, ready-to-use form of a layout, namespaces will cascade down and be applied to all nested fields.

> LocusZoom.Layouts.get('data_layer', 'intervals', { namespace: { intervals: 'kenneth_data' }})`
{ 
  ...omitted, 
  id_field: "kenneth_data:start" 
}

Let me know if this addresses your issue? Otherwise I'll close this sometime after ~ 1 week.

We've mostly weeded out core LZ usages of "bare" fields (eg start vs {{namespace[intervals]}}start), which means that fragment-based matches are fairly unlikely in the sample scenario above. If other edge cases arise, I'm happy to revisit.

kennethbruskiewicz commented 3 years ago

Using LocusZoom.Layouts.get doesn't quite do it for me, because currently I want to be able to create the layout, then namespace it at a later time.LocusZoom.Layouts.get couples the two steps.

I notice that LocusZoom.Layouts.get uses a helper function applyNamespaces. Is it OK if this function is exposed and I use it for this purpose? Or is that an anti-pattern?

abought commented 3 years ago

I notice that LocusZoom.Layouts.get uses a helper function applyNamespaces. Is it OK if this function is exposed and I use it for this purpose? Or is that an anti-pattern?

To be frank, I think you're still trying to hack around the core mechanisms in an imperative style, and somewhat avoiding the declarative/plugin based things that are core to LZ's design.

I'd point you towards the documentation section about "abstract" layouts (written in terms of namespaces, {{{{namespace[assoc]}}log_pvalue) vs concrete layouts (written in terms of specific datasources, kenneth_study:log_pvalue). https://statgen.github.io/locuszoom/docs/guides/rendering_layouts.html#working-with-layouts

Essentially: LocusZoom is designed to be user extensible (this is why it has all those registries with methods named .add and .get). When you talk about wanting to "create the layout, then namespace it at a later time", you're describing the layout registry: this is exactly what happens when you reuse an existing premade layout like LocusZoom.Layouts.get('panel', 'association', namespace: { assoc: 'kenneth_study' }).

You can add your own layout once when the app is started, using LocusZoom.Layouts.add(type, name, kenneth_layout), then reuse it everywhere else using Layouts.get(...), just like you would any built in feature. (that is the entire purpose of the plugin based design: your custom additions can use the same approach and style of code as built in stuff).

You can also customize an existing layout, then add the modified version to the registry.

const base = LocusZoom.Layouts.get('data_layer', 'association_pvalues', { unnamespaced: true }); // keep it abstract. Returns a clean copy (no shared references with the original)
base.x_axis.field = '{{namespace[assoc]}}position2';

LocusZoom.Layouts.add('data_layer', 'kenneth_association', base);

I'm also going to reiterate a standing request for specific feedback on doc improvements: given that many of these questions are a rehash of prior discussions (and that some form of the topics is covered in docs), it's not clear to me that the current reference material is serving its intended purpose. I've considered adding another guide on the subject of adding user features / plugins, but I really can't justify that time if the current docs aren't being used as a starting point. It's a definite catch-22 situation.