document group & bin

[Fil]

for #1343

TODO:

• this is already very long (too long)
• add comments on using the sort{ fy: "x", limit: 10} input option.
• reducer scope? (as in transforms/bin)
• fix types for the {reduce()} function (& create test plots with an explicit scope)
• bin
• reducer label (? mentioned in the comments) (see More support for labels with channel reducers and values #1364)
• tests for reducer & scale (see Reducer implementations are confused with reducer scale overrides #1363)
• test {x1,x2…} parameter for the bin reducer function
• apply changes from fix reducer scale override confusion #1367 (reduceIndex)

[Fil]

I've just added an incomplete commit that shows an underlying type bug with ChannelReducers. Not sure how to solve this.

The incomplete test is in test/plots/penguin-island-mode.ts; it fails because our types aren't certain that the reduce function is called with two arguments (index, values), due to the reducer being allowed to be a ReducerFunction function(values: any[]) => any and being reused in the {reduce: T} construct of ChannelReducers.

[mbostock]

The incomplete test is in test/plots/penguin-island-mode.ts; it fails because our types aren't certain that the reduce function is called with two arguments

That’s only true in this branch because you changed the definition of ReducerImplementation. In the main branch TypeScript seems to infer that it’s a ReducerImplementation:

But, I do think there is ambiguity and likely a bug here introduced by #1310 (to fix #1309). Take this code:

Plot.groupX({y: "count", fill: {reduce: f}}, {x: "species", fill: "island"})

In this case, it’s hard to tell whether the fill output channel is being specified as

1. a reducer with a scale override, in which case f is a ReducerFunction (or a ReducerImplementation object)
2. a reducer implementation, in which case f is a ReducerImplementation["reduce"]

Currently we assume (1)

plot/src/transforms/group.js

Line 181 in 2ac08e2

if (isObject(reduce) && typeof reduce.reduce !== "function") (scale = reduce.scale), (reduce = reduce.reduce);

and if you want a reducer implementation, you would need another level of nesting to get (2):

Plot.groupX({y: "count", fill: {reduce: {reduce: f}}}, {x: "species", fill: "island"})

but this should be considered a bug since we didn’t intend to change the interpretation. Hmm, tricky! I suppose we could look at the length of the reduce function to disambiguate but that could be brittle.

[mbostock]

I wonder if we should make the map/reducer implementations much less ambiguous by using a symbol, e.g., {[Plot.reduce]: …} instead of {reduce: …} to declare an implementation. That would be possible like so:

export declare const reduce: unique symbol;

export interface ReducerImplementation {
  [reduce](index: number[], values: any[]): any;
}

It wouldn’t be backwards compatible, though… 🤔

[Fil]

I don't think we do (1) if f is a function. The new tests reducerBinScopeNone and reducerGroupScopeNone target this particular use case.

It seems like a bug that {reduce: ƒ, scale} is not implemented — only {reduce: "name", scale} works. I'm creating an issue (#1363).

There was a comment about the reducer label but it wasn't implemented; I fixed that and added a test, README docs and type. Maybe this would be better in a separate PR, though, since it's sort-of a new feature? (issue tracked at #1364)

[mbostock]

Yes, please don’t make the label change in this documentation PR. It’s more subtle than it looks and is now delaying adding the documentation.

[mbostock]

The changes to the Reducer types aren’t right.

A ReducerImplementation is an implementation; it can only be an object with a reduce method that takes index and values (plus a context if there’s a scope, and an extent in the case of the bin transform). A ReducerImplementation is never a named built-in reducer name, and a ReducerImplementation cannot declare a scale.

The only place where you can declare a scale override is in the context of a ChannelReducers object.

Also, the BinReducer types are specific to the bin transform, and therefore belong in src/transforms/bin; not sure why you moved them to src/reducer.

I think it’s okay to propose some small type changes along with documentation, but this is bigger than I was expecting. Changes to the types and lots of tests should be done separately from documentation. Also, I intentionally avoided documenting the reducer scope before because it’s very rarely used and adds complexity. I’m not against including scope in the types if we can figure it out, but if we can’t, I definitely think we should cut it because we already have a huge amount of work just to do the documentation, and the types are useful as-is.

[Fil]

Got it—I'm reverting all this. I was following my comment above, but it took me too far :)

Also this seems to work, but I wouldn't know how to express it as an interface:

export type ReducerImplementation =
  | {
      reduce(index: number[], values: any[]): any;
    }
  | {
      reduce(index: number[], values: any[], summary?: any): any;
      scope: "data" | "facet";
    };

export type BinReducerImplementation =
  | {
      reduce(index: number[], values: any[], extent: {x1: any; y1: any; x2: any; y2: any}): any;
    }
  | {
      reduce(index: number[], values: any[], summary?: any, extent?: {x1: any; y1: any; x2: any; y2: any}): any;
      scope: "data" | "facet";
    };