Skip to content

Convert arrayOk attribute to calcdata in calc step #1257

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

Merged
merged 2 commits into from
Dec 22, 2016

Conversation

etpinard
Copy link
Contributor

This PR makes calc-transforms behave as desired on relayout (e.g. zoom).

Moreover, it should also be a perf boost as trace array to calcdata is now called only in the calc as opposed to the plot step (which is called more often e.g. on zoom).

Now, this PR moved around some pretty ancient code. I'm a little afraid about possible regressions. I'm not 100% confident in our test suite for the various restyle call that depend on arraysToCalcdata, I'll dig into our test suites to verify.

- making calc-transforms behave as desired on relayout (e.g. zoom)
- this should also be a perf boost as trace array to calcdata
  is now called only in the calc as-opposed to the plot step
  (which is called more often e.g. on zoom)
@etpinard etpinard added status: in progress bug something broken labels Dec 16, 2016
@rreusser
Copy link
Contributor

I like the concept, but I'd really need to dig in and use it to fully appreciate and understand the true implications of the changes.

One small note on carpet: the smoothing should debatably be view-dependent, which would mean it makes a big difference whether some parts are called in the plot or calc step. This sounds fine in that context; the only point being that I'm very interested in this despite not fully understanding the implications.

@alexcjohnson
Copy link
Collaborator

Looks reasonable to me. Sometime we need to have a discussion about what an explicitly designed pipeline should look like in the context of updates; but until then seems like this kind of change (accumulating as many tests along the way as possible) is about the only way.

@etpinard
Copy link
Contributor Author

will be part of 1.21.3 set to be released in the week of Jan 2.

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

Successfully merging this pull request may close these issues.

3 participants