-
Notifications
You must be signed in to change notification settings - Fork 2k
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 (3/8): CPUAggregator #8888
base: x/aggregation-2
Are you sure you want to change the base?
Conversation
2362a0d
to
430a347
Compare
bf5e17e
to
795f733
Compare
795f733
to
78cfdbc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some first impressions, will come back again for full review.
import {VertexAccessor, evaluateVertexAccessor} from './vertex-accessor'; | ||
|
||
/** Settings used to construct a new CPUAggregator */ | ||
export type CPUAggregatorSettings = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This mix of ...Settings
and ...Props
here kind of trips me up.
- Can it be avoided?
- Or can we work on the naming to make it clearer?
- Could
Props
orOptions
be used as constructor arguments, rather thanSettings
. The latter seems more unfamiliar to me, but maybe used more extensively in deck.gl than I realize.
* If dimensions>1, bin ID should be an array with [dimensions] elements; | ||
* The data point will be skipped if bin ID is null. | ||
*/ | ||
getBin: VertexAccessor<number | number[] | null, any>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Always make it an array for cleaner types? Or there is a big enough convenience in allowing a single number?
*/ | ||
getBin: VertexAccessor<number | number[] | null, any>; | ||
/** Accessor to map each data point to a weight value, defined per channel */ | ||
getWeight: VertexAccessor<number>[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getChannelValue()
?
export type CPUAggregationProps = AggregationProps & {}; | ||
|
||
export type Bin = { | ||
id: number | number[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Plural?
id: number | number[]; | |
ids: number | number[]; |
attributes: {} | ||
}; | ||
|
||
protected getBinId: CPUAggregatorSettings['getBin']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: probably clearer to type out the callback types here, rather than referencing another type.
For #7457
Change List
CPUAggregator