Heatmap contour refactor #1223
Heatmap contour refactor #1223
Conversation
|
ping @alexcjohnson @etpinard No big rush, but nothing here is particularly offensive to me so I'm currently building off of this in my carpet/scattercarpet/contourcarpet trace types (just a heads up 😄 ) |
| @@ -20,6 +18,10 @@ var colorscaleCalc = require('../../components/colorscale/calc'); | |||
| var hasColumns = require('./has_columns'); | |||
| var convertColumnXYZ = require('./convert_column_xyz'); | |||
| var maxRowLength = require('./max_row_length'); | |||
| var cleanData = require('./clean_data'); | |||
There was a problem hiding this comment.
I would prefer not calling this module clean_data.
Trace modules with #cleanData methods are called during the Plotly.plot pipeline here. At the moment, only Scatter has a cleanData method but still adding a heatmap/clean_data.js module that has nothing to do with cleanData step could potentially be confusing.
I'd vote for clean_z.js, but @rreusser I'll let decide on an alternative file name.
There was a problem hiding this comment.
Oh I see, you generalized cleanZ to non- 'z' data arrays.
So maybe clean_2d_array.js instead?
| // neighborCount is the count of 4 nearest neighbors that DO exist | ||
| // this is to give us an order of points to evaluate for interpolation. | ||
| // if no neighbors exist, we iteratively look for neighbors that HAVE | ||
| // neighbors, and add a fractional neighborCount |
There was a problem hiding this comment.
let's bump that comment block above the function statement.
|
|
||
| module.exports = function makeBoundArray(trace, arrayIn, v0In, dvIn, numbricks, ax) { | ||
| var arrayOut = [], | ||
| isContour = Registry.traceIs(trace, 'contour'), |
There was a problem hiding this comment.
non- ⛔ , but it would be nice to remove those Registry.traceIs calls in makeBoundArray and have two (many more) boolean options instead: withCenteredBricks (true for contour and gl2d traces) and startAtZero (for histogram2d traces and categorical axes)
That way makeBoundArray would no longer need trace as an argument
There was a problem hiding this comment.
Agreed. If it's alright, I wanted to keep this PR (almost) entirely just trivial file reorganization so that small but meaningful changes to get too lost in gigantic line-count PRs. In other words, ideally I'd like to push that change off until subsequent PRs that continue to aim for small changes.
There was a problem hiding this comment.
No problem! I was just writing this down so that we don't forget about it.
|
💃 thanks! |
Unless something has gone wrong, this PR makes absolutely no changes to the functioning of the codebase. Instead, it separates some of the contour and heatmap logic into separate files.
In one place I've made marginally more substantial change change:
becomes:
since
cleanDataotherwise has no particular dependence on the variable being thezproperty. There is no change to the behavior though. It simply moves in the direction of being able to apply some of the same functions to, say,trace.yfor carpet plots. There will be more of those changes, but the goal here is to keep things simple since this first step involves moving large chunks of code. Otherwise this PR should be (to the very best of my knowledge) harmless.