Refactoring of the parallel-coordinates element. #2223
Conversation
|
This PR should also take care of b/129365646 (errorneous brush behavior when linear-scale / log-scale axis contains just one point). |
|
Thanks for the PR! I’m OOO until next Wednesday and won’t be able to |
|
@stephanwlee , @wchargin PTAL. |
tensorboard/plugins/hparams/tf_hparams_query_pane/tf-hparams-query-pane.html
Outdated
Show resolved
Hide resolved
...plugins/hparams/tf_hparams_scale_and_color_controls/tf-hparams-scale-and-color-controls.html
Outdated
Show resolved
Hide resolved
| result = schema.hparamColumns.length + schema.metricColumns.findIndex( | ||
| c => (c.metricInfo.name === metricName)); | ||
| } | ||
| console.assert(result !== undefined); |
There was a problem hiding this comment.
AFAICT, result cannot be undefined. Is this strictly required?
There was a problem hiding this comment.
It's a sanity check. I agree that it's currently clear that it can't happen, but it may not be so clear after the code evolves in future changes. I typically use asserts to check assumptions (and document them) on the internal logic of the program. So these are typically conditions that one doesn't expect to see fail.
There was a problem hiding this comment.
it may not be so clear after the code evolves in future changes.
This reminds me of saying that goes like 'don't design for tomorrow's problem'.
I typically use asserts to check assumptions (and document them) on the internal logic of the program
Especially since this is not an input validation, that'd be a job of a test, not some assertion.
There was a problem hiding this comment.
I think my intended use of asserts here is not clear. Let me try to clarify:
I use asserts to both verify and document invariants on the internal logic of the code that I believe must hold regardless of user's input. If an invariant does not hold, there is a bug and the code needs to be changed to fix it. I use asserts here as I would use a "CHECK" macro in (google) C++ or "Preconditions.CheckState" in Java. For example, to check that an enum's value is one of its predefined members or that an API is being used correctly (open is called on a file before we try to access it). I think assert shouldn't be called for checking a condition on (some function of) the user's input or to check that some external API didn't return an error (like a file wasn't opened successfully)
So when an invariant on the code's logic fails, I'd log as much information as possible for the developer to allow reproducing the bug and also notify the user that it happened. The user can then file a bug report with the above information so that the bug can be fixed.
This reminds me of saying that goes like 'don't design for tomorrow's problem'.
Especially since this is not an input validation, that'd be a job of a test, not some assertion.
Like tests, these assertions safeguard against future changes that would break past assumptions. But, they document these assumptions in a much more direct fashion (one reads the code and immediately sees them), they require a lot less overhead to write, and they run during a real execution of the program so they are more helpful in fixing bugs. The downside obviously, is that they have a performance overhead. Also, of course, I'm not suggesting we should use assertions instead of tests, but I just claim these asserts have their place as well.
Having said all that, in this particular case, I don't feel strongly about having the assert there, since the distance between where 'result' is computed and the assertion is very small and so it's clear from the code that it holds. I agree you could argue that it's overkill, so I'm removing it.
There was a problem hiding this comment.
I really appreciate the change. Thanks!
I still have not yet completed by first pass. Before I complete it, let me ask this: would it be possible to break tf-hparams-parallel-coords-plot.ts into few files? I don't think you will need to do tons of type refactoring. Preferably file per big item (Interaction vs. Axis. vs. Line) would be nice.
| this._axesManager.dragEnd(/*transitionDuration=*/ 500); | ||
| this._linesManager.recomputeControlPoints(LineType.Foreground, | ||
| /* transitionDuration=*/ 500); | ||
| window.setTimeout(() => { |
There was a problem hiding this comment.
You may want to use this event instead :)
https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/transitionend_event
There was a problem hiding this comment.
Thanks, I didn't know about that. I agree that doing this with the event is a more explicit, but I'd rather not fix something that's not really broken. Also this would make the code dependent on D3 implementing SVG transitions using CSS transitions.
tensorboard/plugins/hparams/tf_hparams_parallel_coords_plot/tf-hparams-parallel-coords-plot.ts
Outdated
Show resolved
Hide resolved
tensorboard/plugins/hparams/tf_hparams_parallel_coords_plot/tf-hparams-parallel-coords-plot.ts
Outdated
Show resolved
Hide resolved
| * scale types. | ||
| */ | ||
| export enum ScaleType { | ||
| Linear = "LINEAR", |
There was a problem hiding this comment.
ENUMs tend to be ALL_CAPS.
Are you using these string values somewhere? If not, just use whatever TypeScript gives you.
export enum ScaleType {
LINEAR,
LOGARITHMIC,
...
}There was a problem hiding this comment.
Changed to ALL_CAPS. I am setting these to the strings so that they can be freely interchanged with the current (pre-typescript) values I already use in options.columns[].scale.
|
|
||
| private _lower: number; | ||
| private _upper: number; | ||
| private _lowerOpen: boolean |
There was a problem hiding this comment.
stylistic nit: missing semi-colon.
Also, can you put the properties near the top of the class? Thanks!
There was a problem hiding this comment.
Hmm. Style guides for other languages, like C++, typically call for putting the private parts of a class at the end, so that it's easier to read the public interface (which I guess is read more than the private one). What's your reasoning here for having the private properties first?
| if (sessionGroupSel === undefined) { | ||
| sessionGroupSel = d3.selectAll(null); | ||
| } | ||
| console.assert(sessionGroupSel.size() <= 1); |
There was a problem hiding this comment.
so when sessionGroupSel is undefined, this assertion fails, right? If so, why is it an optional parameter?
There was a problem hiding this comment.
Note that when sessionGroupSel is undefined, it's set in line 640 to be the empty selection which would pass the assertion.
| } | ||
|
|
||
| /** | ||
| * @returns the sessionGroup object this handle references or null if |
There was a problem hiding this comment.
Instead of mixing Closure type with TypeScript, I would make the TypeScript type more concrete.
There was a problem hiding this comment.
So I'm not sure I understand. I actually didn't know that I was using a "closure type" -- I was simply trying to follow the TypeScript doc convention I found here:
https://typedoc.org/guides/doccomments/
Unfortunately, I don't have a 'SessionGroup' type definition so I don't write 'SessionGroup | null' here.
One could add it, but the downside is we'll need to remember to change it every time we change the underlying protocol buffer (SessionGroup is a JavaScript representation of the proto defined in api.proto obtained using proto3's proto to JSON mapping).
But I may have misundestood your intention. Can you clarify what you mean by concrete type here?
| * dragging an axis and contains the logic for re-ordering the axes | ||
| * during dragging. | ||
| */ | ||
| class AxesManager { |
There was a problem hiding this comment.
Instead of calling this a manager, I would choose a name like AxesGroup or something like that. Word util or manager are often used to mean a place where random functions end up. Since this is also responsible for rendering axes, I would call it something more appropriate.
There was a problem hiding this comment.
I changed to AxesCollection and LinesCollection. I think AxesGroup is fine, but LinesGroup sounded a bit ambigious for me and I wanted the same suffix for both Axes and Lines. What do you think?
|
|
||
| // Update the DOM. | ||
| this._parentsSel = this._parentsSel | ||
| .data(Array.from(visibleColIndices), /*key=*/ (colIndex => colIndex)); |
There was a problem hiding this comment.
Is the key part strictly required? It looks too simple for that.
There was a problem hiding this comment.
Yes. Without it, D3 joins by array order:
"If a key function is not specified, then the first datum in data is assigned to the first selected element, the second datum to the second selected element, and so on.",
whereas here if a DOM element from the previous state represented column 5 (say), I'd like to continue to use it to represent column 5, and not change it to some other column.
tensorboard/plugins/hparams/tf_hparams_parallel_coords_plot/tf-hparams-parallel-coords-plot.ts
Outdated
Show resolved
Hide resolved
I can certainly do that. I don't think it's a big change. I do sometimes find it easier to read through code of related classes when they are all in one file; however, I agree that here this file of ~1000 lines is too big. |
|
Thanks @stephanwlee! PTAL. |
|
BTW. I'm stil not a 100% used to github's flow of review, so I may miss some of your comments. If I left any unreplied, please just remind me in your response. |
| result = schema.hparamColumns.length + schema.metricColumns.findIndex( | ||
| c => (c.metricInfo.name === metricName)); | ||
| } | ||
| console.assert(result !== undefined); |
There was a problem hiding this comment.
it may not be so clear after the code evolves in future changes.
This reminds me of saying that goes like 'don't design for tomorrow's problem'.
I typically use asserts to check assumptions (and document them) on the internal logic of the program
Especially since this is not an input validation, that'd be a job of a test, not some assertion.
tensorboard/plugins/hparams/tf_hparams_parallel_coords_plot/lines.ts
Outdated
Show resolved
Hide resolved
| this._linesCollection.selectedSessionGroupHandle().sessionGroup()); | ||
| } | ||
|
|
||
| // Polymer adds an extra ".tf-hparams-parallel-coords-plot" class to |
There was a problem hiding this comment.
This is unique to Polymer1 (or shadyCSS). The correct way to do this is to use https://polymer-library.polymer-project.org/1.0/api/classes/Polymer.Base#method-scopeSubtree.
After making DOM modification, Polymer class has to invoke this method with the new DOM as the first arg.
| * object control which axes are visible. | ||
| */ | ||
| public updateAxes(options: any, sessionGroups: any[]) { | ||
| console.assert(!this.isAxisDragging()); |
There was a problem hiding this comment.
Instead of throwing exception, I'd silently exit or say something more user friendly especially if it is going to litter in the console.
There was a problem hiding this comment.
See also my reply to comment: https://github.com/tensorflow/tensorboard/pull/2223/files#r284474857
Note that console.assert() doesn't throw an exception:
function f() { console.assert(0);console.log("foo"); }
f()
VM371:1 Assertion failed: console.assert /* Note that there's a stack trace here that I removed. */
VM371:1 foo
undefined
As for the message, I use asserts as checks that verify assumptions the code makes about its internal logic. Violations of these are always bugs that are not meant to be recovered from and need to be fixed. If this was an offline batch program, the reasonable course of action would be to terminate the program (fail fast). If this was a server, the reasonable course of action would be to return an "internal server error" to the client and stop handling the current request. Since this is a UI, I don't want to do either, but I do want to document the fact that the bug happened. Thus, the assertion failure message should contain a stack trace, the condition that failed and any other information that can help debugging. It's mostly for the developer not the end-user.
tensorboard/plugins/hparams/tf_hparams_parallel_coords_plot/axes.ts
Outdated
Show resolved
Hide resolved
tensorboard/plugins/hparams/tf_hparams_parallel_coords_plot/axes.ts
Outdated
Show resolved
Hide resolved
tensorboard/plugins/hparams/tf_hparams_parallel_coords_plot/axes.ts
Outdated
Show resolved
Hide resolved
| this._stationaryAxesPositions.domain( | ||
| /* TypeScript doesn't allow spreading a Set, so we convert to an | ||
| Array first. */ | ||
| newDomain.concat(...Array.from(visibleColIndices))); |
There was a problem hiding this comment.
Separately, I am a bit confused by Line 508 and this line.
Line 506: newDomain = visibileColIndices ∩ this._stationaryAxesPositions.domain()
Line 508: visibleColIndices = visibileColIndices - newDomain
Line 512: newDomain = newDomain ∪ visibleColIndices = originalVisibileColIndices
It might be my afternoon drowsiness but I must be missing something (or can't comprehend code).
There was a problem hiding this comment.
You're understanding this right. I do it this way rather than setting the domain to visibleColIndices directly since
I want to preserve the order of the existing visible axes. I rewrote the comment to make it clearer. Please let me know if that makes more sense now.
tensorboard/plugins/hparams/tf_hparams_parallel_coords_plot/axes.ts
Outdated
Show resolved
Hide resolved
tensorboard/plugins/hparams/tf_hparams_parallel_coords_plot/axes.ts
Outdated
Show resolved
Hide resolved
| * object control which axes are visible. | ||
| */ | ||
| public updateAxes(options: any, sessionGroups: any[]) { | ||
| console.assert(!this.isAxisDragging()); |
There was a problem hiding this comment.
See also my reply to comment: https://github.com/tensorflow/tensorboard/pull/2223/files#r284474857
Note that console.assert() doesn't throw an exception:
function f() { console.assert(0);console.log("foo"); }
f()
VM371:1 Assertion failed: console.assert /* Note that there's a stack trace here that I removed. */
VM371:1 foo
undefined
As for the message, I use asserts as checks that verify assumptions the code makes about its internal logic. Violations of these are always bugs that are not meant to be recovered from and need to be fixed. If this was an offline batch program, the reasonable course of action would be to terminate the program (fail fast). If this was a server, the reasonable course of action would be to return an "internal server error" to the client and stop handling the current request. Since this is a UI, I don't want to do either, but I do want to document the fact that the bug happened. Thus, the assertion failure message should contain a stack trace, the condition that failed and any other information that can help debugging. It's mostly for the developer not the end-user.
| this._stationaryAxesPositions.domain( | ||
| /* TypeScript doesn't allow spreading a Set, so we convert to an | ||
| Array first. */ | ||
| newDomain.concat(...Array.from(visibleColIndices))); |
There was a problem hiding this comment.
You're understanding this right. I do it this way rather than setting the domain to visibleColIndices directly since
I want to preserve the order of the existing visible axes. I rewrote the comment to make it clearer. Please let me know if that makes more sense now.
| this._linesCollection.selectedSessionGroupHandle().sessionGroup()); | ||
| } | ||
|
|
||
| // Polymer adds an extra ".tf-hparams-parallel-coords-plot" class to |
tensorboard/plugins/hparams/tf_hparams_parallel_coords_plot/lines.ts
Outdated
Show resolved
Hide resolved
| result = schema.hparamColumns.length + schema.metricColumns.findIndex( | ||
| c => (c.metricInfo.name === metricName)); | ||
| } | ||
| console.assert(result !== undefined); |
There was a problem hiding this comment.
I think my intended use of asserts here is not clear. Let me try to clarify:
I use asserts to both verify and document invariants on the internal logic of the code that I believe must hold regardless of user's input. If an invariant does not hold, there is a bug and the code needs to be changed to fix it. I use asserts here as I would use a "CHECK" macro in (google) C++ or "Preconditions.CheckState" in Java. For example, to check that an enum's value is one of its predefined members or that an API is being used correctly (open is called on a file before we try to access it). I think assert shouldn't be called for checking a condition on (some function of) the user's input or to check that some external API didn't return an error (like a file wasn't opened successfully)
So when an invariant on the code's logic fails, I'd log as much information as possible for the developer to allow reproducing the bug and also notify the user that it happened. The user can then file a bug report with the above information so that the bug can be fixed.
This reminds me of saying that goes like 'don't design for tomorrow's problem'.
Especially since this is not an input validation, that'd be a job of a test, not some assertion.
Like tests, these assertions safeguard against future changes that would break past assumptions. But, they document these assumptions in a much more direct fashion (one reads the code and immediately sees them), they require a lot less overhead to write, and they run during a real execution of the program so they are more helpful in fixing bugs. The downside obviously, is that they have a performance overhead. Also, of course, I'm not suggesting we should use assertions instead of tests, but I just claim these asserts have their place as well.
Having said all that, in this particular case, I don't feel strongly about having the assert there, since the distance between where 'result' is computed and the assertion is very small and so it's clear from the code that it holds. I agree you could argue that it's overkill, so I'm removing it.
Done. |
|
@stephanwlee, friendly ping. |
...plugins/hparams/tf_hparams_scale_and_color_controls/tf-hparams-scale-and-color-controls.html
Outdated
Show resolved
Hide resolved
| this.schema.metricColumns[metricIndex].displayed = | ||
| this._metrics[metricIndex].displayed; | ||
| this._updateVisibleSchema(); | ||
| this.notifyPath(`schema.metricColumns.${metricIndex}.displayed`); |
There was a problem hiding this comment.
I believe if you use set API, you don't need to notify like this.
this.set(`schema.metricColumns.${metricIndex}.displayed`, this._metrics[metricIndex].displayed);(Also, instead of imperatively calling updateVisibleSchema, make sure there is an observer that, on metricColumn display changes, trigger updateVisibleSchema.
Similar concern above for _hparamVisibilityChanged.
There was a problem hiding this comment.
The issue here is with atomically modifying schema so that observing elements will see the changes to both visibleSchema and metricColumns together. If I use the 'set' API then the notification for the changed metricColumns property will go out immediately, and an observing element will see a schema object in an incosistent state--one whose visibleSchema does not match the new 'metricColumn' information. This may create problems: for example, it would make the assumptions that the code in tf.hparam.utils.getAbsoluteColumnIndex() not hold.
I know this is not ideal, but I don't know of a better solution. I tried to explain that in the comment above the code. I'm open to suggestions.
There was a problem hiding this comment.
Ah, that makes sense and shame on me for not reading all the comments carefully. Sorry about that.
While your rationale makes sense, it looks rather brittle or unconventional. How about this?
this.schema = this._computeSchema();Is this unappealing for any performance reason that I am not seeing? btw, if above works, I would remove both _metricVisibilityChanged and _hparamVisibilityChanged to this new method, _updateSchema.
There was a problem hiding this comment.
While your rationale makes sense, it looks rather brittle or unconventional. How about this?
I agree. I don't like it very much either, and I'd be happy with a better solution.
Using: 'this.schema = this._computeSchema()' will replace the identity of the schema object each time the user changes the visibility of a single column. This makes it harder for for observing elements to redraw only the changed part. See also the comments in lines 345--350. For example, if the schema object stays the same and we only change its properties, the vaadin-grid element can set the column property 'hidden' to be linked to the (negation of) the corresponding 'displayed' subproperty of schema.hparamColumn or schema.metricColumn.
There was a problem hiding this comment.
If the point of the PR is a refactor, I would advise against such performance optimization (without carefully looking at what is costing the most CPU cycle; i.e., premature optimization). Also, there are many other ways to mitigate such issue while keeping your code sensible.
One small gotcha with this approach was, which I found while poking around with your branch, with current code, both 1 and 2 while 3 does not (missing *; i.e., schema.*. Without 3 getting triggered, InteractionManager can go out of sync with the options which fortuitously did not happen because we were mutating schema by reference. However, while I could not trigger it, I think they could really go out of sync soon (depending on execution order) when 4 happens. The point here is that, while the bug I pointed out is solvable, it just makes everything more brittle and harder to reason without concerning the greater system.
| visibleColIndices = new Set<number>(visibleColIndices); | ||
| let newDomain: number[] = this._stationaryAxesPositions.domain().filter( | ||
| colIndex => visibleColIndices.has(colIndex)); | ||
| newDomain.forEach(colIndex => visibleColIndices.delete(colIndex)); |
There was a problem hiding this comment.
I don't think comments are panacea and making code a little bit readable will help. Please take a look at code below and tell me what you think.
// Sort visibleColIndices so indicies present in _stationaryAxesPositions
// appear before others.
const visibleDomain = this._stationaryAxesPositions.domain()
.filter(colIndex => visibleColIndices.has(colIndex));
const newDomain = Array.from(new Set([
...Array.from(visibleDomain),
...Array.from(visibleColIndices),
]);
this._stationaryAxesPositions.domain(newDomain);There was a problem hiding this comment.
Your suggestion does look more readable. Done.
| () => { | ||
| const _this = this; | ||
| this._fgPathsSel.each( | ||
| function(sessionGroup) { |
There was a problem hiding this comment.
Might work: you can write below to document what this means.
function(this: SVGPathElement, sessionGroup) {
...
}
...plugins/hparams/tf_hparams_scale_and_color_controls/tf-hparams-scale-and-color-controls.html
Outdated
Show resolved
Hide resolved
| visibleColIndices = new Set<number>(visibleColIndices); | ||
| let newDomain: number[] = this._stationaryAxesPositions.domain().filter( | ||
| colIndex => visibleColIndices.has(colIndex)); | ||
| newDomain.forEach(colIndex => visibleColIndices.delete(colIndex)); |
There was a problem hiding this comment.
Your suggestion does look more readable. Done.
| () => { | ||
| const _this = this; | ||
| this._fgPathsSel.each( | ||
| function(sessionGroup) { |
| this.schema.metricColumns[metricIndex].displayed = | ||
| this._metrics[metricIndex].displayed; | ||
| this._updateVisibleSchema(); | ||
| this.notifyPath(`schema.metricColumns.${metricIndex}.displayed`); |
There was a problem hiding this comment.
The issue here is with atomically modifying schema so that observing elements will see the changes to both visibleSchema and metricColumns together. If I use the 'set' API then the notification for the changed metricColumns property will go out immediately, and an observing element will see a schema object in an incosistent state--one whose visibleSchema does not match the new 'metricColumn' information. This may create problems: for example, it would make the assumptions that the code in tf.hparam.utils.getAbsoluteColumnIndex() not hold.
I know this is not ideal, but I don't know of a better solution. I tried to explain that in the comment above the code. I'm open to suggestions.
|
Thanks @stephanwlee! PTAL. |
erzel
force-pushed
the
par_coords_refactor
branch
2 times, most recently
from
a3a7838 to
3431826
Compare
|
Hi @stephanwlee, I've finally managed to incoporate the changes we talked about. I'm sorry for the delay, I'm in a conference this week. PTAL. Thanks! |
erzel
force-pushed
the
par_coords_refactor
branch
from
3431826 to
9f2c9df
Compare
| * hparamColumns: Array<{hparamInfo: HParamInfo}>, | ||
| * metricColumn: Array<{metricInfo: MetricInfo>}>, | ||
| * } | ||
| * columnsVisibility: Array<Boolean>, |
There was a problem hiding this comment.
The array here can be very confusing. Instead, may I suggest, maybe in a follow up, use an object where it is keyed by name of the column (assuming that they are unique? -- I cannot imagine it not being unique as it will confuse user too :))
Otherwise, this user of this prop has to be aware that hparam columns appear before metric ones.
Alternative would be, there being flat columns array and have a separate map/list of hparam & metrics column ids
There was a problem hiding this comment.
I've been using the convention of indexing columns by their index when the hparams appear before the metrics throughout the code. See for example the existing comment in tf-hparams-utils.html lines 52--63. So I think it would actually be easier to understand the code if I'm consistent here.
| this._hparams[hparamIndex].displayed; | ||
| this._updateVisibleSchema(); | ||
| this.notifyPath(`schema.hparamColumns.${hparamIndex}.displayed`); | ||
| _hparamVisibilityChanged() { |
There was a problem hiding this comment.
Instead of this, I would recommend using idiomatic Polymer: http://go/gh/erzel/tensorboard/pull/2/files#diff-c777620714a33699c59f411f55c718c4R545
Also, consider debouncing at least a microtask :)
| // in the schema will be displayed. See tf-hparams-query-pane for | ||
| // the definition of this object. | ||
| schema: { | ||
| // The visibleSchema defining the columns to use. |
There was a problem hiding this comment.
super nit: the line wrapping is weird :\
There was a problem hiding this comment.
Thanks Stephan!
PTAL.
One weird thing I discovered is that for some reason the Vulcanizer (closure?) decides to drop the parameter from a d3.brushSelection(parameter) I make in line 650 of tf-hparams-scatter-plot-matrix-plot.html. Which causes a bug.
To overcome this I'm fooling it by writing the call as: d3["brushSelection"](parameter). Is there a better way, or maybe you know why this is happening all of a sudden?
Erez.
| * hparamColumns: Array<{hparamInfo: HParamInfo}>, | ||
| * metricColumn: Array<{metricInfo: MetricInfo>}>, | ||
| * } | ||
| * columnsVisibility: Array<Boolean>, |
There was a problem hiding this comment.
I've been using the convention of indexing columns by their index when the hparams appear before the metrics throughout the code. See for example the existing comment in tf-hparams-utils.html lines 52--63. So I think it would actually be easier to understand the code if I'm consistent here.
| const brushSelection = d3.brushSelection(cellGNode); | ||
| // For some reason the closure compiler drops the argument when we | ||
| // write the call below as 'd3.brushSelection(cellGNode)'. | ||
| const brushSelection = d3["brushSelection"](cellGNode); |
There was a problem hiding this comment.
No action required. As you have indicated, I couldn't find the reason why Closure deletes the argument from this method by inspecting both d3 code and playing around with defining type of this method. I am okay with this hack for time being.
Optional: Btw, although it is technically not in scope of this work, lines 628/639/642 have argument that are not used.
There was a problem hiding this comment.
Thanks! I removed the arguments.
Could you merge the PR, if you don't have any more comments?
Common typo catcher caught few things in our recent PR tensorflow#2223. Closure type system annotates return type with "@return" instead of "@returns".
preserved under the deprecated field 'visibleSchema' of 'schema'. The goal is eventually to make everyone use 'schema' and to remove 'visibleSchema'.