Skip to content

Plotly.animate doesn't unset attributes #1533

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

Closed
n-riesco opened this issue Mar 30, 2017 · 10 comments
Closed

Plotly.animate doesn't unset attributes #1533

n-riesco opened this issue Mar 30, 2017 · 10 comments

Comments

@n-riesco
Copy link
Contributor

n-riesco commented Mar 30, 2017

Here's a codepen that illustrates the issue.

I expected that:

    Plotly.animate(gd, {
        traces: [0],
        data: [{
            "marker.opacity": null
        }]
    }, {
        transition: {
            easing:"cubic-in-out",
            duration: 500        
        }
    });

would unset `gd.data[0].marker.opacity.

@rreusser
Copy link
Contributor

rreusser commented Mar 30, 2017

Can you clarify the precise result you see or expect to see? It seems to me like it's working:

ezgif-2-f9cb5bd875

Here's a minimal codepen that queries it directly: http://codepen.io/rsreusser/pen/ZemVqQ?editors=0010 :

Plotly.plot(gd, 
  [{y: [1, 2, 3], marker: {opacity: 0.5}}]
).then(function () {
  console.log('Before animation: marker.opacity =', gd._fullData[0].marker.opacity)
  
  return Plotly.animate(gd,
    [{data: [{'marker.opacity': null}]}]
  )
  
}).then(function () {
  console.log('After animating to null: marker.opacity =', gd._fullData[0].marker.opacity)
})

In the console I see:

Before animation: marker.opacity = 0.5
After animating to null: marker.opacity = 1

@n-riesco
Copy link
Contributor Author

@rreusser I expected gd.data[0].opacity to be unset. Instead it's set to null. Note that your example prints the value from gd._fullData[0].

@rreusser
Copy link
Contributor

Thanks. I understand now. I guess I considered the behavior functionally equivalent, but I could be wrong about that. Just to help plan the best course of action, can you clarify whether this breaks things—as opposed to being an inconvenience which I agree should perhaps be changed?

@rreusser
Copy link
Contributor

rreusser commented Mar 30, 2017

The relevant line is in Plots.extendObjectWithContainers#L1614:

dest = Lib.extendDeepNoArrays(dest || {}, expandedObj);

Since it's a deep extend, it does not automatically unset null properties. The challenge is that we would need extendDeepNoArraysAndUnsetNulls. It might be the right answer, but it suggests maybe we need to step back and think for a moment. ping @etpinard for second opinion.

@n-riesco
Copy link
Contributor Author

It doesn't break anything for me at the moment. I've only noticed that Plotly.validate(gd.data, gd.layout) would return an error.

@etpinard
Copy link
Contributor

etpinard commented Mar 30, 2017

@n-riesco @rreusser this situation is similar to #1410. I have no strong opinion here to be honest. I can think of situation where pruning make sense and others that don't. Better yet, maybe as @n-riesco suggested yesterday we should expose some .getAttribute method on Plotly where it would be easier to guarantee the result as opposed to making users dig into gd.data / gd._fullData etc ...

Moreover, @n-riesco's point about Plotly.validate is fair. Maybe we should make Plotly.validate skip keys linked to nulls entirely.

@rreusser
Copy link
Contributor

rreusser commented Mar 30, 2017

Might be reiterating a point from that ticket, but to clarify and apart from validation, is there ever a case where null has a meaningful effect different from the attribute not being present?
Related: @bpostlethwaite points out that the workspace logic for examining the settability of an attribute is:

// an option is not settable if it's either:
// - missing (so is irrelevant given other settings)
// - an array (so contains data that you don't want to override)
function notSettable(val, arrayOk) {
    return val===undefined || (Array.isArray(val) && !arrayOk);
}

(i.e. null is settable, right?)

@etpinard
Copy link
Contributor

Is this really a big deal? Should we do anything about it?

@etpinard
Copy link
Contributor

Quick update: almost all of the pruning logic got 🔪 in @alexcjohnson 's e84d4b9

@etpinard
Copy link
Contributor

Closing. This doesn't seem to cause any important problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants