-
Notifications
You must be signed in to change notification settings - Fork 85
Conversation
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.
Looks great. Nice work Yannick. Left a few comment, we can discuss tomorrow.
Reviewed 16 of 16 files at r1.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @tafsiri)
karma.conf.js, line 20 at r1 (raw file):
const karmaTypescriptConfig = { tsconfig: 'tsconfig.json', reports: {},
any reason for removing this settings?
README.md, line 187 at r1 (raw file):
## render.table: (data: {headers: [], values: [][]}, container: Surface|HTMLElement) => void
Here and elsewhere in this file: remove the :
in between, i.e. render.table(data:....) => void
README.md, line 198 at r1 (raw file):
* data.headers are the column names * data.values is an array of arrays (one for each row). The inner array length usually matches the length of data.headers. Usually
expand a bit on what if the inner length doesn't match the length of headers.
README.md, line 202 at r1 (raw file):
content so html strings are also supported. * @param container An HTMLElement or Surface in which to draw the table.
wrap HTMLElement and Surface in `` so markdown renders them as code.
README.md, line 209 at r1 (raw file):
## render.histogram: (data: [], container: Surface|HTMLElement, opts: {}) => void Renders a Histogram
add a period
README.md, line 213 at r1 (raw file):
* @param data Data in the following format, (an array of objects) * [ {value: number}, ... ] * @param container An HTMLElement or Surface in which to draw the histogram
wrap in ``
README.md, line 223 at r1 (raw file):
own stats as in some cases they may be able to compute them more efficiently. Stats should have the following format
add :
at the end
src/types.ts, line 140 at r1 (raw file):
export type Drawable = HTMLElement|Surface|{ drawArea: HTMLElement; [others: string]: {}|null|undefined;
sorry I didn't notice this in the previous PR. Why do you need to have the key lookup signature here?
src/render/barchart.ts, line 30 at r1 (raw file):
* @param data Data in the following format, (an array of objects) * [ {index: number, value: number} ... ] * @param container An HTMLElement or Surface in which to draw the bar chart.
wrap HtmlElement and Surface in ``- here and elsewhere.
src/render/histogram.ts, line 19 at r1 (raw file):
* * @param data Data in the following format, (an array of objects) * [ {value: number}, ... ]
for large array of numbers, would be more efficient for the user to pass TypedArray. WDYT?
src/render/histogram.ts, line 26 at r1 (raw file):
* @param opts.maxBins maximimum number of bins to use in histogram * @param opts.stats summary statistics to show. These will be computed * internally if no stats are passed. Pass `false` to not
style guide: here and elsewhere, indent 4 spaces for each consecutive line of the same param.
src/render/table.ts, line 32 at r1 (raw file):
* values: any[][], * } * data.headers are the column names
General note: I can see how keeping the two docs (README and jsdoc) in sync might become an issue in the future.
src/render/table.ts, line 38 at r1 (raw file):
* content so html strings are also supported. * * @param container An HTMLElement or Surface in which to draw the table.
wrap in ``
src/util/dom.ts, line 7 at r1 (raw file):
import {Drawable} from '../types'; const defaultSubsurfaceOpts = {
make this ALL_CAPS
src/util/math.ts, line 30 at r1 (raw file):
let numNans = 0; for (let i = 0; i < numVals; i++) {
hmm, why not call tensorStats(tensor(input)) and avoid code duplication?
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.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @tafsiri)
karma.conf.js, line 20 at r1 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
any reason for removing this settings?
With the setting present i get much worse stack traces when there is an error (as if source maps aren't hooked up)
README.md, line 187 at r1 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
Here and elsewhere in this file: remove the
:
in between, i.e.render.table(data:....) => void
Done
README.md, line 198 at r1 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
expand a bit on what if the inner length doesn't match the length of headers.
Done
README.md, line 202 at r1 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
wrap HTMLElement and Surface in `` so markdown renders them as code.
Done
README.md, line 209 at r1 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
add a period
Done
README.md, line 213 at r1 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
wrap in ``
Done
README.md, line 223 at r1 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
add
:
at the end
Done
src/types.ts, line 140 at r1 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
sorry I didn't notice this in the previous PR. Why do you need to have the key lookup signature here?
Removed and works. I must have refactored out whatever caused it to fail previously
src/render/barchart.ts, line 30 at r1 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
wrap HtmlElement and Surface in ``- here and elsewhere.
Done
src/render/histogram.ts, line 19 at r1 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
for large array of numbers, would be more efficient for the user to pass TypedArray. WDYT?
Yes i do think it would be nice to take a regular array (or typed array). Currently vega will internally turn that into an array of objects with a 'data' element (which i think is unfortunate). So while this kind of exposes an internal implementation detail it also means there won't be two separate representations of the data (and thus using more memory anyway).
There is a part of me that wants to hide that detail as i don't see this data object getting any richer in future.
Thoughts? Maybe we can discuss in person, its something i went back and forth on.
src/render/histogram.ts, line 26 at r1 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
style guide: here and elsewhere, indent 4 spaces for each consecutive line of the same param.
Done
src/render/table.ts, line 32 at r1 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
General note: I can see how keeping the two docs (README and jsdoc) in sync might become an issue in the future.
Yeah. I think investigating doc generation for this is something I could down the line.
src/render/table.ts, line 38 at r1 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
wrap in ``
Done
src/util/dom.ts, line 7 at r1 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
make this ALL_CAPS
Done
src/util/math.ts, line 30 at r1 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
hmm, why not call tensorStats(tensor(input)) and avoid code duplication?
Hmm i think the main goal here is to not go to the GPU for some of these ops as they are not necessarily easily parallelized. Also some of the stuff needs to happen on CPU even in tensorStats so i end up looping through the values anyway.
With more benchmarking i'd be up for removing one of these (and delegating to whichever is faster). I actually probably lean towards calling .data
and delegating to the arrayStats impl
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.
LGTM!
Reviewed 4 of 6 files at r2.
Reviewable status: 14 of 16 files reviewed, all discussions resolved (waiting on @dsmilkov)
src/render/histogram.ts, line 19 at r1 (raw file):
Previously, tafsiri (Yannick Assogba) wrote…
Yes i do think it would be nice to take a regular array (or typed array). Currently vega will internally turn that into an array of objects with a 'data' element (which i think is unfortunate). So while this kind of exposes an internal implementation detail it also means there won't be two separate representations of the data (and thus using more memory anyway).
There is a part of me that wants to hide that detail as i don't see this data object getting any richer in future.
Thoughts? Maybe we can discuss in person, its something i went back and forth on.
I vote for number[]|TypedArray
. The simpler the better. If new version of vega all of a sudden allows typedarray and is much faster, we can change that later without sacrificing API or performance.
src/util/math.ts, line 30 at r1 (raw file):
Previously, tafsiri (Yannick Assogba) wrote…
Hmm i think the main goal here is to not go to the GPU for some of these ops as they are not necessarily easily parallelized. Also some of the stuff needs to happen on CPU even in tensorStats so i end up looping through the values anyway.
With more benchmarking i'd be up for removing one of these (and delegating to whichever is faster). I actually probably lean towards calling
.data
and delegating to the arrayStats impl
Got it. Fine with keeping them both for now - add a todo to revisit this.
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.
Reviewable status: 8 of 16 files reviewed, all discussions resolved (waiting on @dsmilkov)
src/render/histogram.ts, line 19 at r1 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
I vote for
number[]|TypedArray
. The simpler the better. If new version of vega all of a sudden allows typedarray and is much faster, we can change that later without sacrificing API or performance.
I added number[]|TypedArray to the existing type
src/util/math.ts, line 30 at r1 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
Got it. Fine with keeping them both for now - add a todo to revisit this.
Done
This change is![Reviewable](https://camo.githubusercontent.com/23b05f5fb48215c989e92cc44cf6512512d083132bd3daf689867c8d9d386888/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)