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

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

Merged
merged 4 commits into from
Jun 10, 2024

Conversation

Pessimistress
Copy link
Collaborator

@Pessimistress Pessimistress commented May 12, 2024

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

coveralls commented May 12, 2024

Coverage Status

coverage: 89.672% (-0.2%) from 89.901%
when pulling 0dc11d4 on x/aggregation-1
into 49131c1 on master.

Copy link
Collaborator

@donmccurdy donmccurdy left a comment

Choose a reason for hiding this comment

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

I like this direction a lot! 👏🏻

Left a few questions, nothing critical.

Copy link
Collaborator

@ibgreen ibgreen left a comment

Choose a reason for hiding this comment

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

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.


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

export type AggregationProps = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: TSDoc on the type as well.

@@ -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 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We have limited slots (255) in picking and including composite layers could be breaking to tiled use cases.

Copy link
Collaborator

@ibgreen ibgreen left a comment

Choose a reason for hiding this comment

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

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
Copy link
Collaborator Author

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

data: LayerDataSource<DataT>;
};

export default abstract class AggregationLayer<
Copy link
Collaborator

Choose a reason for hiding this comment

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

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)

@Pessimistress Pessimistress merged commit 9351848 into master Jun 10, 2024
2 checks passed
@Pessimistress Pessimistress deleted the x/aggregation-1 branch June 10, 2024 00:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants