GPU Aggregation (1/8): Aggregator and AggregationLayer #8886

Pessimistress · 2024-05-12T02:56:43Z

For #7457

The goals of this refactor:

Unified Aggregator interface for both GPU aggregation and CPU aggregation
Simplified Aggregator and AggregationLayer classes that no longer concern:
- Different props.data formats (iterable, non-iterable, binary...)
- Projection systems
- Specific prop names and update triggers
Fully typed and extensively documented

Planned PRs:

Aggregator and AggregationLayer
GPUAggregator
CPUAggregator
ScreenGridLayer
GridLayer
HexagonLayer
ContourLayer
Remove legacy code

Change List

New Aggregator interface
New AggregationLayer abstract class
Add isDrawable to Layer class to decouple isComposite from drawing behavior. AggregationLayer is a composite layer (renders sublayers) but also participate in the draw cycle.

coveralls · 2024-05-12T03:19:09Z

coverage: 89.578% (-0.2%) from 89.821%
when pulling af9e3ff on x/aggregation-1
into b5a6313 on master.

donmccurdy

I like this direction a lot! 👏🏻

Left a few questions, nothing critical.

donmccurdy · 2024-05-14T01:54:14Z

modules/aggregation-layers/src/aggregation-layer-v9/aggregation-layer.ts

I'm working through the PRs in order, apologies if these questions would be answered by a later PR. :)

donmccurdy · 2024-05-14T01:54:30Z

modules/aggregation-layers/src/aggregation-layer-v9/aggregation-layer.ts

+  state!: {
+    gpuAggregator: GPUAggregator | null;
+    cpuAggregator: CPUAggregator | null;
+  };


Question – It appears that you plan for both aggregators to have similar public APIs, does the layer need the binary distinction CPU and GPU aggregators as two different props?

Exactly, I would expect there to be only an aggregator prop here.

donmccurdy · 2024-05-14T01:54:38Z

modules/aggregation-layers/src/aggregation-layer-v9/aggregation-layer.ts

+  /** Called to create a GPUAggregator instance */
+  abstract getGPUAggregator(): GPUAggregator | null;
+  /** Called to create a CPUAggregator instance if getGPUAggregator() returns null */
+  abstract getCPUAggregator(): CPUAggregator | null;


Question - If this creates an instance rather than returning an already-assigned one, should it be prefixed 'createFoo' or 'makeFoo' etc.? Is there any assumption that this will only be called once? No objection if you prefer the 'get' prefix here, just wondering.

Hmm... Why would we wan't to create aggregator in callback as opposed to have it passed in as a prop from the subclass?

donmccurdy · 2024-05-14T02:23:58Z

modules/aggregation-layers/src/aggregation-layer-v9/aggregator.ts

+ * - The number of data points
+ * - The group that each data point belongs to, by mapping each data point to a _binId_ (integer or array of integers)
+ * - The value(s) to aggregate, by mapping each data point in each channel to one _weight_
+ * - The method (_aggregationOperation_) to reduce a list of _weights_ to one number, such as SUM


Minor – I felt like the term "weights" was a bit misleading in the previous iteration of aggregation... For me it implies that they are coefficients for something, rather than a bunch of numbers we're going to sum or average. I think "values" or "inputs" might work just as well here without that confusion?

Without detailed knowledge, but based on your comment, I would vote for values. I would definitely expect weights to be an extra set of parameters used to rebalance things, not the values themselves.
(Though it can depend on perspective: e.g. in a graph, edges typically have weights not values.)

donmccurdy · 2024-05-14T02:24:48Z

modules/aggregation-layers/src/aggregation-layer-v9/aggregator.ts

+ * - The aggregated values (_result_) as a list of numbers for each channel, comprised of one number per bin
+ * - The [min, max] among all aggregated values (_domain_) for each channel
+ *
+ */


Thanks for adding this summary! 🙏🏻

ibgreen

Very nice. Being able to reason about and define Aggregators separately and have a layer that accepts them follows the true spirit of vis.gl composability

I would mostly encourage another pass to remove differences between GPUAggregator and CPUAggregator from being visible in the AggregationLayer. I think you are almost there, see detailed notes.

ibgreen · 2024-05-14T09:56:05Z

modules/aggregation-layers/src/aggregation-layer-v9/aggregation-layer.ts

+import {Aggregator} from './aggregator';
+
+// TODO
+type GPUAggregator = Aggregator & {destroy(): void};


Nit: Why keep destroy() outside of the interface? Making it part of the interface (possibly optional if CPUAggregator just can't be bothered to implement it) would seem to make more sense to me.

ibgreen · 2024-05-14T09:56:58Z

modules/aggregation-layers/src/aggregation-layer-v9/aggregation-layer.ts

+  state!: {
+    gpuAggregator: GPUAggregator | null;
+    cpuAggregator: CPUAggregator | null;
+  };


Exactly, I would expect there to be only an aggregator prop here.

ibgreen · 2024-05-14T09:58:06Z

modules/aggregation-layers/src/aggregation-layer-v9/aggregation-layer.ts

+  /** Called to create a GPUAggregator instance */
+  abstract getGPUAggregator(): GPUAggregator | null;
+  /** Called to create a CPUAggregator instance if getGPUAggregator() returns null */
+  abstract getCPUAggregator(): CPUAggregator | null;


Hmm... Why would we wan't to create aggregator in callback as opposed to have it passed in as a prop from the subclass?

ibgreen · 2024-05-14T10:00:45Z

modules/aggregation-layers/src/aggregation-layer-v9/aggregation-layer.ts

+    if (params.changeFlags.extensionsChanged) {
+      this.state.gpuAggregator?.destroy();
+      this.state.gpuAggregator = this.getGPUAggregator();
+      if (this.state.gpuAggregator) {


Nit: probably better "ask" the aggregator instead of such switching on aggregator "type".

Along the lines of.

if (this.state.aggregator.usesAttributeManager) {

ibgreen · 2024-05-14T10:01:49Z

modules/aggregation-layers/src/aggregation-layer-v9/aggregation-layer.ts

+  // Override Layer.finalizeState to dispose the GPUAggregator instance
+  finalizeState(context: LayerContext) {
+    super.finalizeState(context);
+    this.state.gpuAggregator?.destroy();


Nit: IMHO, it would look nicer to call destroy() on both here, even if it was a no-op on the cpuAggregator.

ibgreen · 2024-05-14T10:15:49Z

modules/aggregation-layers/src/aggregation-layer-v9/aggregator.ts

+
+export type AggregationOperation = 'SUM' | 'MEAN' | 'MIN' | 'MAX';
+
+export type AggregationProps = {


Nit: TSDoc on the type as well.

ibgreen · 2024-05-14T10:16:43Z

modules/aggregation-layers/src/aggregation-layer-v9/aggregator.ts

+};
+
+/**
+ * _Aggregation_ is a 2-step process:


Nit: Maybe start with a more general sentence about the aggregator (perhaps being a base class etc) before diving into the details of the algorithm?

ibgreen · 2024-05-14T10:18:45Z

modules/aggregation-layers/src/aggregation-layer-v9/aggregator.ts

+ * - The number of data points
+ * - The group that each data point belongs to, by mapping each data point to a _binId_ (integer or array of integers)
+ * - The value(s) to aggregate, by mapping each data point in each channel to one _weight_
+ * - The method (_aggregationOperation_) to reduce a list of _weights_ to one number, such as SUM


Without detailed knowledge, but based on your comment, I would vote for values. I would definitely expect weights to be an extra set of parameters used to rebalance things, not the values themselves.
(Though it can depend on perspective: e.g. in a graph, edges typically have weights not values.)

ibgreen · 2024-05-14T10:19:38Z

modules/aggregation-layers/src/aggregation-layer-v9/aggregator.ts

+  getResultDomain(channel: number): [min: number, max: number];
+
+  /** Returns the information for a given bin. */
+  getBin(index: number): {


Nit: Maybe worth breaking out as type for the Bin or AggregatedBin or ...

ibgreen · 2024-05-14T10:20:32Z

modules/core/src/lib/composite-layer.ts

@@ -41,6 +41,11 @@ export default abstract class CompositeLayer<PropsT extends {} = {}> extends Lay
    return true;
  }

+  /** `true` if the layer renders to screen */
+  get isDrawable(): boolean {


Nit: Is this extra wrinkle worth it? Should we just start calling draw on composite layers and just make sure we provide an empty implementation?

ibgreen

and extensively documented

The Aggregator class deserves extensive user facing documentation, to help end-users define their own aggregators and that could probably be copy pasted from your voluminous comments in the source.

Pessimistress · 2024-05-14T13:53:24Z

@ibgreen I will add markdown documentation once the API discussion settles. I didn't want to write them prematurely as you can't type check those.

felixpalmer · 2024-05-16T09:29:12Z

modules/aggregation-layers/src/aggregation-layer-v9/aggregation-layer.ts

+  static layerName = 'AggregationLayer';
+
+  state!: {
+    gpuAggregator: GPUAggregator | null;


I wonder if rather than specifying CPU/GPU as options, the layer could instead accept a list of aggregators, and then pick whichever is compatible (similar to how luma does with the Device). We may want to add support for a webGpuAggregator in addition to webGlAggregator, or who knows what else in the future

felixpalmer · 2024-05-16T09:29:37Z

modules/aggregation-layers/src/aggregation-layer-v9/aggregator.ts

@@ -0,0 +1,65 @@
+import type {Attribute, BinaryAttribute} from '@deck.gl/core';
+
+export type AggregationOperation = 'SUM' | 'MEAN' | 'MIN' | 'MAX';


Worth adding 'COUNT'? For the GPU aggregation case it would avoid uploading a buffer containing just 1s

felixpalmer · 2024-05-16T09:37:30Z

modules/aggregation-layers/src/aggregation-layer-v9/aggregation-layer.ts

+  data: LayerDataSource<DataT>;
+};
+
+export default abstract class AggregationLayer<


Have you thought about how this could work with a TileLayer? I'm working on a cluster layer right now which works with a tiled data source, and aggregates whenever the visible data changes (outputting a single ScatterplotLayer)

Aggregator and AggregationLayer

134a139

Pessimistress requested review from felixpalmer, donmccurdy and ibgreen May 12, 2024 02:56

explicitly create AttributeManager

af9e3ff

Pessimistress force-pushed the x/aggregation-1 branch from c354c6e to af9e3ff Compare May 12, 2024 03:12

donmccurdy approved these changes May 14, 2024

View reviewed changes

ibgreen approved these changes May 14, 2024

View reviewed changes

ibgreen reviewed May 14, 2024

View reviewed changes

felixpalmer approved these changes May 16, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GPU Aggregation (1/8): Aggregator and AggregationLayer #8886

GPU Aggregation (1/8): Aggregator and AggregationLayer #8886

Pessimistress commented May 12, 2024 •

edited

coveralls commented May 12, 2024

donmccurdy left a comment

donmccurdy May 14, 2024

donmccurdy May 14, 2024

ibgreen May 14, 2024

donmccurdy May 14, 2024

ibgreen May 14, 2024

donmccurdy May 14, 2024

ibgreen May 14, 2024

donmccurdy May 14, 2024

ibgreen left a comment

ibgreen May 14, 2024

ibgreen May 14, 2024

ibgreen May 14, 2024

ibgreen May 14, 2024

ibgreen May 14, 2024

ibgreen May 14, 2024

ibgreen May 14, 2024

ibgreen May 14, 2024

ibgreen May 14, 2024

ibgreen May 14, 2024

ibgreen left a comment

Pessimistress commented May 14, 2024

felixpalmer May 16, 2024

felixpalmer May 16, 2024

felixpalmer May 16, 2024


		export type AggregationOperation = 'SUM' \| 'MEAN' \| 'MIN' \| 'MAX';

		export type AggregationProps = {

		@@ -0,0 +1,65 @@
		import type {Attribute, BinaryAttribute} from '@deck.gl/core';

		export type AggregationOperation = 'SUM' \| 'MEAN' \| 'MIN' \| 'MAX';

GPU Aggregation (1/8): Aggregator and AggregationLayer #8886

Are you sure you want to change the base?

GPU Aggregation (1/8): Aggregator and AggregationLayer #8886

Conversation

Pessimistress commented May 12, 2024 • edited

Change List

coveralls commented May 12, 2024

donmccurdy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibgreen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibgreen left a comment

Choose a reason for hiding this comment

Pessimistress commented May 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pessimistress commented May 12, 2024 •

edited