graph: fix 'ungroup this series' button #4817
Conversation
| const readonlySeriesMap = new Map([ | ||
| ['fooNode', tf_graph.SeriesGroupingType.UNGROUP], | ||
| ['barNode', tf_graph.SeriesGroupingType.GROUP], | ||
| ]); |
There was a problem hiding this comment.
idea: another way to have this test would be to Object.freeze it.
There was a problem hiding this comment.
Interesting idea, although I don't know of any way to freeze a Map without writing a custom FrozenMap.
| hierarchy: Hierarchy, | ||
| hierarchyParams: HierarchyParams | ||
| ) { | ||
| export function getIncompatibleOps(hierarchy: Hierarchy) { |
There was a problem hiding this comment.
would you mind specifying the return type? :D
| } | ||
| @computed('graphHierarchy', 'hierarchyParams') | ||
| @computed('graphHierarchy') | ||
| get _incompatibleOpNodes(): object { |
There was a problem hiding this comment.
if you type the getIncompatibleOps, you can better type the return types.
| hierarchy.getSeriesGroupType(seriesNode.name) === | ||
| tf_graph.SeriesGroupingType.GROUP |
There was a problem hiding this comment.
Previous code reads !(seriesNode.name in map) and it makes me think that this code is basically checking to see if the hierarchy.getSeriesGroupType(seriesNode.name) has the default value in case it does not exist. I think it is more prudent to null to Map.prototype.has than matching against the default value defined in various places in this module.
Would it not read better if the hierarchy.getSeriesGroupType(seriesNode.name) were to return null?
There was a problem hiding this comment.
I would still prefer to hide the fact that the group types are stored implicitly, because it is a concern that is specific to Hierarchy internals. External consumers of the Hierarchy actually do not need to know what null values mean, so exposing only Group/Ungroup keeps consumers less complex and easier to reason about, imo.
While the previous code does check whether a node has been explicitly grouped/ungrouped, the new code matches the same intent (if a grouped series is too short, then ungroup it), and does not require readers to understand anything about the default group type.
I've updated the comment above on L900 accordingly.
2 participants
TensorBoard detects series of nodes (e.g. 'add_1', 'add_2', 'add_3', ...)
and visually collapses them into a single "series node". Currently, the
"ungroup this series" button in the right, floating info pane is broken.
Vestiges indicate that at one time, the button would modify the
HierarchyParams.seriesMapobject, and rebuild a newHierarchyusing the modified
HierarchyParams.This change properly wires up the handlers, but instead of modifying
the
seriesMapin place, it creates a fresh newseriesMapand newHierarchyParamsfor the newHierarchyto use. This avoids a bugwhere ungrouping a series then selecting a different graph to view
would cause the stale
seriesMapfrom the previous graph to carryover.
Fixes #4732
To test manually, feel free to check out this sample graph with a series:
https://github.com/tensorflow/tensorboard/files/6086986/series_graph.txt
Added a unit test and manually checked that the button now works.
