-
Notifications
You must be signed in to change notification settings - Fork 64
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
Improve Typing for Aesthetics and Transforms #46
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,8 @@ import { | |
scaleOrdinal, | ||
scaleSequential, scaleSequentialLog, | ||
scaleSequentialPow, | ||
ScaleContinuousNumeric, | ||
ScaleIdentity, | ||
} from 'd3-scale'; | ||
import { rgb } from 'd3-color'; | ||
import * as d3Chromatic from 'd3-scale-chromatic'; | ||
|
@@ -16,7 +18,8 @@ import type { | |
TextureSet | ||
} from './AestheticSet'; | ||
import { isOpChannel, isLambdaChannel, | ||
isConstantChannel } from './types'; | ||
isConstantChannel, | ||
Transform} from './types'; | ||
import type { | ||
OpChannel, LambdaChannel, | ||
Channel, BasicChannel, ConstantChannel, | ||
|
@@ -27,7 +30,10 @@ import { Vector } from 'apache-arrow'; | |
import { StructRowProxy } from 'apache-arrow/row/struct'; | ||
import { QuadTile } from './tile'; | ||
|
||
const scales : { [key : string] : Function } = { | ||
const scales: Record< | ||
Transform, | ||
(range?: Iterable<number>) => ScaleContinuousNumeric<number, number, never> | ScaleIdentity<never> | ||
> = { | ||
sqrt: scaleSqrt, | ||
log: scaleLog, | ||
linear: scaleLinear, | ||
|
@@ -98,19 +104,17 @@ function okabe() { | |
|
||
okabe(); | ||
|
||
type Transform = 'log' | 'sqrt' | 'linear' | 'literal'; | ||
|
||
abstract class Aesthetic { | ||
public abstract default_range : [number, number]; | ||
public abstract default_constant : number | [number, number, number]; | ||
public abstract default_transform : 'log' | 'sqrt' | 'linear' | 'literal'; | ||
public abstract default_transform : Transform | ||
public scatterplot : Scatterplot; | ||
public field: string | null = null; | ||
public regl : Regl; | ||
public _texture_buffer : Float32Array | Uint8Array | null = null; | ||
public _domain : [number, number]; | ||
public _range : [number, number] | Uint8Array; | ||
public _transform : 'log' | 'sqrt' | 'linear' | 'literal' | undefined; | ||
public _transform : Transform | undefined; | ||
public dataset : QuadtileSet; | ||
public partner : Aesthetic | null = null; | ||
public _textures : Record<string, Texture2D> = {}; | ||
|
@@ -149,14 +153,11 @@ abstract class Aesthetic { | |
} | ||
|
||
get transform() { | ||
if (this._transform) return this._transform; | ||
return this.default_transform; | ||
return this._transform || this.default_transform; | ||
} | ||
|
||
set transform(transform) { | ||
|
||
this._transform = transform; | ||
|
||
} | ||
|
||
get scale() { | ||
|
@@ -165,9 +166,9 @@ abstract class Aesthetic { | |
return r.charAt(0).toUpperCase() + r.slice(1); | ||
} | ||
|
||
let scale = scales[this.transform]() | ||
.domain(this.domain) | ||
.range(this.range); | ||
const domain = scales[this.transform]() | ||
.domain(this.domain) as ScaleContinuousNumeric<number, number, never>; | ||
let scale = domain.range(this.range); | ||
|
||
const range = this.range; | ||
|
||
|
@@ -688,15 +689,16 @@ export const dimensions = { | |
y : Y | ||
}; | ||
|
||
type concrete_aesthetics = X | | ||
Y | | ||
Size | | ||
Jitter_speed | | ||
Jitter_radius | | ||
Color | | ||
Filter; | ||
type ConcreteAesthetic = | ||
| X | ||
| Y | ||
| Size | ||
| Jitter_speed | ||
Comment on lines
+692
to
+696
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's up with this formatting, especially the line before the first X? I'm conditioned to think of leading lines as guard rather than an OR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's the same formatting that OCaml uses - I prefer it for multi-line union types as it keeps everything consistent. |
||
| Jitter_radius | ||
| Color | ||
| Filter; | ||
|
||
export abstract class StatefulAesthetic<T extends concrete_aesthetics> { | ||
export abstract class StatefulAesthetic<T extends ConcreteAesthetic> { | ||
// An aesthetic that tracks two states--current and last. | ||
// The point is to handle transitions. | ||
// It might make sense to handle more than two states, but there are | ||
|
@@ -775,7 +777,7 @@ class StatefulColor extends StatefulAesthetic<Color> { get Factory() { return Co | |
class StatefulFilter extends StatefulAesthetic<Filter> { get Factory() { return Filter; }} | ||
class StatefulFilter2 extends StatefulAesthetic<Filter> { get Factory() { return Filter; }} | ||
|
||
export const stateful_aesthetics : Record<string, StatefulAesthetic<typeof Aesthetic>> = { | ||
export const stateful_aesthetics = { | ||
x : StatefulX, | ||
x0 : StatefulX0, | ||
y : StatefulY, | ||
|
@@ -787,6 +789,7 @@ export const stateful_aesthetics : Record<string, StatefulAesthetic<typeof Aesth | |
filter : StatefulFilter, | ||
filter2 : StatefulFilter2 | ||
}; | ||
export type StatefulAestheticSet = typeof stateful_aesthetics; | ||
|
||
function parseLambdaString(lambdastring : string) { | ||
// Materialize an arrow function from its string. | ||
|
@@ -856,4 +859,4 @@ function lambda_to_function(input : LambdaChannel) : (d : any) => number { | |
//@ts-ignore | ||
const func : (d : any) => number = new Function(arg, code); | ||
return func; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -126,7 +126,7 @@ export class ReglRenderer extends Renderer { | |
const { prefs } = this; | ||
const { transform } = this.zoom; | ||
const { aes_to_buffer_num, buffer_num_to_variable, variable_to_buffer_num } = this.allocate_aesthetic_buffers(); | ||
console.log(prefs.arrow_table); | ||
// console.log(prefs.arrow_table); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Feel free to just kill anything that isn't a console.warn or console.error. I would lint them out of existence except there are few cases that they're actually intentional, alas. |
||
const props = { | ||
// Copy the aesthetic as a string. | ||
aes: { encoding: this.aes.encoding }, | ||
|
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 is nice, glad to see this signature clarified.