Skip to content
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

Redesign "materialization" of data specs #10235

Merged
merged 25 commits into from Jul 9, 2020
Merged

Conversation

mattpap
Copy link
Contributor

@mattpap mattpap commented Jun 27, 2020

This PR achieves several goals:

  1. All data initialization is now explicit, i.e. no guessing in super-classes what properties a glyph may implement.
  2. What can be a typed array is a typed array. This is to allow almost zero-copy data transfer between threads.
    • Nested arrays are stored as flat RaggedArrays (typed arrays with offsets into subarrays).
    • Colors are stored as packed 32-bit integers.
  3. Transforms are computed in-place whenever it is allowed.

@bryevdv
Copy link
Member

bryevdv commented Jul 2, 2020

@mattpap How close is this to being ready? I would like to start in earnest on some WebGL experiments but this work definitely intersects relevant codepaths. I can branch of of this if you think it will be awhile (but then it would be nice if force-pushes can be avoided)

for (let i = 0; i < n; i++) {
const xi = x[i]
const yi = y[i]
const [merc_xi, merc_yi] = wgs84_mercator.compute(xi, yi)
merc_x[i] = merc_xi
merc_y[i] = merc_yi
}
}

export function $project_xsys(xs: Arrayable<number>[], ys: Arrayable<number>[], merc_xs?: Arrayable<number>[], merc_ys?: Arrayable<number>[]): void {
Copy link
Member

@bryevdv bryevdv Jul 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the $ signify non-allocating?

Copy link
Contributor Author

@mattpap mattpap Jul 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, though I have mixed feelings about this notation.

Copy link
Contributor Author

@mattpap mattpap Jul 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@bryevdv bryevdv Jul 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@mattpap
Copy link
Contributor Author

mattpap commented Jul 2, 2020

How close is this to being ready?

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.

@mattpap
Copy link
Contributor Author

mattpap commented Jul 3, 2020

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.).

@bryevdv
Copy link
Member

bryevdv commented Jul 3, 2020

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).


export class XCoordinateSpec extends CoordinateSpec { readonly dimension = "x" }
export class YCoordinateSpec extends CoordinateSpec { readonly dimension = "y" }

export class XCoordinateSeqSpec extends CoordinateSeqSpec { readonly dimension = "x" }
export class YCoordinateSeqSpec extends CoordinateSeqSpec { readonly dimension = "y" }

export class XCoordinateSeqSeqSeqSpec extends CoordinateSeqSeqSeqSpec { readonly dimension = "x" }
export class YCoordinateSeqSeqSeqSpec extends CoordinateSeqSeqSeqSpec { readonly dimension = "y" }
Copy link
Member

@bryevdv bryevdv Jul 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@mattpap mattpap Jul 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will add this to the overall "naming things" issue.

ctx.lineJoin = join
ctx.lineCap = cap
ctx.lineDash = dash
ctx.lineDashOffset = offset
Copy link
Member

@bryevdv bryevdv Jul 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With these changes what is the need for explicit intermediate storage here (besides color and alpha)?

@@ -75,7 +76,7 @@ def parameters(cls):
kind=Parameter.POSITIONAL_OR_KEYWORD,

# 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
Copy link
Member

@bryevdv bryevdv Jul 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@mattpap mattpap Jul 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -25,7 +25,7 @@ describe("Bug", () => {
y_range: new DataRange1d(),
})
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})
Copy link
Member

@bryevdv bryevdv Jul 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is this addition for?

Copy link
Contributor Author

@mattpap mattpap Jul 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@bryevdv bryevdv Jul 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, I thought there was supposed to be a default width of 1 for bars?

Copy link
Contributor Author

@mattpap mattpap Jul 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@bryevdv
Copy link
Member

bryevdv commented Jul 6, 2020

@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?

@mattpap
Copy link
Contributor Author

mattpap commented Jul 6, 2020

Graphs are broken, because they circumvent the well defined initialization of glyphs. This seems to be the last thing to fix.

@mattpap mattpap merged commit 06be48a into branch-2.2 Jul 9, 2020
@mattpap mattpap deleted the mattpap/materialize branch Jul 9, 2020
paul-tqh-nguyen pushed a commit to paul-tqh-nguyen/bokeh that referenced this pull request Jul 9, 2020
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants