Redesign "materialization" of data specs #10235
Conversation
|
@mattpap How close is this to being |
bokehjs/src/lib/core/util/projections.ts
Outdated
8000
| } | ||
| } | ||
|
|
||
| export function $project_xsys(xs: Arrayable<number>[], ys: Arrayable<number>[], merc_xs?: Arrayable<number>[], merc_ys?: Arrayable<number>[]): void { |
There was a problem hiding this comment.
Does the $ signify non-allocating?
There was a problem hiding this comment.
Yes, though I have mixed feelings about this notation.
There was a problem hiding this comment.
My preference would be to make those as /** @internal */ (this has meaning in TS, by excluding such entities from generated declaration files), but that would make them untestable in the current setup. However, I've been experimenting with a different approach to compiling tests, in which internal declarations would be usable, so this may get resolved at some point.
There was a problem hiding this comment.
I don't have any strong opinion about the notation. Is there a possibility that extension developers might want to be able to use the non-allocating version?
It's feature ready, but it seems it breaks a few things. I can get this finished next few days, maybe even tomorrow, especially as it blocks my other work as well. |
|
On the other hand, I wonder what kind of changes you expect to be doing? Because, if I was doing this work, I would start from improving/replacing the existing webgl engine, within the existing integration framework, and then work on improving the integration itself. I also need to know the extent of your changes, because this PR is just one in a series of PRs that are going to touch this part of bokehjs (for color mapping, indexing, web workers, etc.). |
@mattpap absolutely, I need to do a little experimentation to get to that point, and I just didn't want to start that process on top of code that is about to change in this PR. To give you an early idea of thoughts though: My main observation about the webgl is simply that the codepath branches off at an wild tangent in an awkward place and starts up a set of tasks that in a completely different structure than the canvas rendering. What would like to do is try to make the rendering steps more granular so that both webgl and and can as rendering can be described by them (though individual steps might end up as be no-ops on one or the other side, e.g. a step to do a data-to-screen mapping would be a no-op on the webgl side, because that mapping will be done in the shader with the gl program executes) But the point is to try to bring canvas and webgl rendering under some conceptual alignment (as much as possible). |
a0bdc0f to
fea6ac4
Compare
| export class YCoordinateSeqSpec extends CoordinateSeqSpec { readonly dimension = "y" } | ||
|
|
||
| export class XCoordinateSeqSeqSeqSpec extends CoordinateSeqSeqSeqSpec { readonly dimension = "x" } | ||
| export class YCoordinateSeqSeqSeqSpec extends CoordinateSeqSeqSeqSpec { readonly dimension = "y" } |
There was a problem hiding this comment.
Just tossing out as an idea: what do you think of something like XCoordinateSeqSpec, XCoordinateSeq2Spec, XCoordinateSeq3Spec? I actually find SeqSeqSeqSpec harded to quickly visually parse
There was a problem hiding this comment.
I will add this to the overall "naming things" issue.
| ctx.lineJoin = join | ||
| ctx.lineCap = cap | ||
| ctx.lineDash = dash | ||
| ctx.lineDashOffset = offset |
There was a problem hiding this comment.
With these changes what is the need for explicit intermediate storage here (besides color and alpha)?
bokeh/models/glyph.py
Outdated
|
|
||
| # for positional arg properties, default=None means no default | ||
| default=Parameter.empty if default is None else default | ||
| default=Parameter.empty if default is None or is_field(default) else default |
There was a problem hiding this comment.
This condition is a bit circumspect, I had to think about it for a bit. Maybe update the comment to explain that "args" that are explicitly specified as field are intended to be required in glyph methods?
There was a problem hiding this comment.
I changed this, so that arguments are processed from last to first. This way first non-default argument will make all following non-default, even if they have defaults (as it is now possible), thus avoiding gaps in positional arguments with and without defaults values.
| }) | ||
| const source = new ColumnDataSource({data: {x: [["a", "b"], ["b", "c"]], y: [1, 2]}}) | ||
| p.vbar({x: {field: "x"}, top: {field: "y"}, source}) | ||
| p.vbar({x: {field: "x"}, top: {field: "y"}, width: 0.1, source}) |
There was a problem hiding this comment.
The test originally depended on null to 0 conversion, and produced zero width bars, which due to non-zero line width appeared to have non-zero width. The more I test bokehjs alone, the more cases like this get revealed.
There was a problem hiding this comment.
I see, I thought there was supposed to be a default width of 1 for bars?
There was a problem hiding this comment.
There is a default, but a single fat bar (of width 1) doesn't seem appropriate for this test. It looks like it's hiding something underneath the bar.
|
@mattpap Lot of great stuff here. I only had a few comments/questions. Do you have an updated ETA? The examples report looks good and the failing tests seem like are not much to fix up? |
|
Graphs are broken, because they circumvent the well defined initialization of glyphs. This seems to be the last thing to fix. |
6fd226d to
82550f5
Compare
82550f5 to
75dcee0
Compare
75dcee0 to
5f1c988
Compare
* Explicitly define glyph coordinates
* Use NumberArray.set() in ImageURL
* Add default values
* Don't check for nulls in MultiLine._index_data()
* Wrap only plain object default values
* Make data projection explicit
* Clean up HexTile._set_data()
* Add support for ragged arrays
* Encode dimension in coordinate specs
* Explicit data materialization
* Update incomplete unit tests
* Update graph layout providers
* Fail safe when a column is missing
* Update incomplete glyph tests
* Assert number arrays in corner cases
* Fix glyphs' defaults
* Explicit defaults in {V,H}Bar glyphs
* Respect packed colors in webgl backend
* Unify handling of colors and alpha
* Use CoordinateSeqSeqSeqSpec for multi polygons
* Make toStringTag static
* Increase pixel allowance
* Respect glyph protocol in graph renderer
* Ignore early glyph positional defaults
* Mark export selenium tests as flaky
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
This PR achieves several goals:
RaggedArrays(typed arrays with offsets into subarrays).