Enable Custom x/y axis values for heatmap#958
Enable Custom x/y axis values for heatmap#958kirandharmar867-maker wants to merge 1 commit intofossasia:masterfrom
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds optional explicit x/y axis value support to visdom.heatmap(), with shape validation and backward-compatible fallbacks to existing column/row name handling. Sequence diagram for visdom.heatmap with explicit x and y axis valuessequenceDiagram
actor User
participant Client as VisdomClient
participant Visdom as Visdom
participant Backend as PlotBackend
User->>Client: heatmap(X, opts with x, y)
Client->>Visdom: heatmap(X, win, env, update, opts)
Visdom->>Visdom: validate rownames length if present
Visdom->>Visdom: validate columnnames length if present
Visdom->>Visdom: assert len(opts.x) == X.shape[1]
Visdom->>Visdom: assert len(opts.y) == X.shape[0]
Visdom->>Visdom: build data
Visdom-->>Backend: data.z, data.x from opts.x, data.y from opts.y
Backend-->>User: Rendered heatmap with custom axes
Class diagram for Visdom heatmap options with new x and y axesclassDiagram
class Visdom {
+heatmap(X, win, env, update, opts)
}
class HeatmapOptions {
+list x
+list y
+list columnnames
+list rownames
+float xmin
+float xmax
}
class HeatmapDataPayload {
+list z
+list x
+list y
+float zmin
+float zmax
+string type
}
Visdom ..> HeatmapOptions : uses
Visdom ..> HeatmapDataPayload : constructs
HeatmapOptions <|-- BackwardCompatibleOptions
class BackwardCompatibleOptions {
+list columnnames
+list rownames
}
HeatmapOptions : x optional
HeatmapOptions : y optional
HeatmapDataPayload : x = opts.x or opts.columnnames
HeatmapDataPayload : y = opts.y or opts.rownames
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new shape validations for
opts['x']andopts['y']useassert, which can be stripped with Python optimization flags; consider raising a concrete exception (e.g.,ValueError) instead to ensure the checks always run. - The error message for the
opts['y']length check reads "number of row in X"; update this to "number of rows in X" for consistency with therownamesassertion message.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new shape validations for `opts['x']` and `opts['y']` use `assert`, which can be stripped with Python optimization flags; consider raising a concrete exception (e.g., `ValueError`) instead to ensure the checks always run.
- The error message for the `opts['y']` length check reads "number of row in X"; update this to "number of rows in X" for consistency with the `rownames` assertion message.
## Individual Comments
### Comment 1
<location> `py/visdom/__init__.py:1883-1885` </location>
<code_context>
+
+ if opts.get("y") is not None:
+ assert len(opts["y"]) == X.shape[0], \
+ "length of y must match number of row in X"
+
data = [
</code_context>
<issue_to_address>
**nitpick (typo):** Fix grammar in the error message for the `y` length assertion.
The message is slightly ungrammatical. Please change it to: `"length of y must match number of rows in X"` to align with the earlier rownames assertion and improve clarity.
```suggestion
if opts.get("y") is not None:
assert len(opts["y"]) == X.shape[0], \
"length of y must match number of rows in X"
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if opts.get("y") is not None: | ||
| assert len(opts["y"]) == X.shape[0], \ | ||
| "length of y must match number of row in X" |
There was a problem hiding this comment.
nitpick (typo): Fix grammar in the error message for the y length assertion.
The message is slightly ungrammatical. Please change it to: "length of y must match number of rows in X" to align with the earlier rownames assertion and improve clarity.
| if opts.get("y") is not None: | |
| assert len(opts["y"]) == X.shape[0], \ | |
| "length of y must match number of row in X" | |
| if opts.get("y") is not None: | |
| assert len(opts["y"]) == X.shape[0], \ | |
| "length of y must match number of rows in X" |
There was a problem hiding this comment.
Pull request overview
Adds support for passing explicit x/y axis values into Visdom.heatmap() so heatmaps can display meaningful, scaled axes instead of implicit pixel indices.
Changes:
- Adds optional
opts["x"]andopts["y"]handling for heatmap axis values. - Validates provided axis lengths against the heatmap shape.
- Preserves legacy behavior by falling back to
columnnames/rownameswhen explicit axes are not provided.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if opts.get("x") is not None: | ||
| assert len(opts["x"]) == X.shape[1], \ | ||
| "length of x must match number of columns in X" | ||
|
|
||
| if opts.get("y") is not None: | ||
| assert len(opts["y"]) == X.shape[0], \ | ||
| "length of y must match number of row in X" |
There was a problem hiding this comment.
The heatmap() docstring lists opts.columnnames/opts.rownames but doesn’t mention the newly supported opts.x/opts.y. Please update the supported opts list (and expected types/shapes) so users discover this feature via help(viz.heatmap).
| "z": nan2none(X.tolist()), | ||
| "x": opts.get("columnnames"), | ||
| "y": opts.get("rownames"), | ||
| "x": opts.get("x", opts.get("columnnames")), | ||
| "y": opts.get("y", opts.get("rownames")), |
There was a problem hiding this comment.
opts.get("x", opts.get("columnnames")) will return None (and override columnnames) if the caller includes the key x with value None. That breaks the intended fallback behavior / backward compatibility. Consider using an explicit if opts.get("x") is not None conditional (same for y) so columnnames/rownames are used when x/y are absent or None.
| assert len(opts["x"]) == X.shape[1], \ | ||
| "length of x must match number of columns in X" | ||
|
|
||
| if opts.get("y") is not None: | ||
| assert len(opts["y"]) == X.shape[0], \ | ||
| "length of y must match number of row in X" |
There was a problem hiding this comment.
opts["x"] / opts["y"] are only length-checked, but not coerced/validated to JSON-serializable types. Since _send uses json.dumps(msg) directly, passing numpy arrays / tensors (a likely use case for axis values) will raise a TypeError: Object of type ndarray is not JSON serializable. Consider validating x/y types and converting to a plain Python list (e.g., np.asarray(...).ravel().tolist()) before constructing data.
| assert len(opts["x"]) == X.shape[1], \ | |
| "length of x must match number of columns in X" | |
| if opts.get("y") is not None: | |
| assert len(opts["y"]) == X.shape[0], \ | |
| "length of y must match number of row in X" | |
| x_vals = np.asarray(opts["x"]).ravel() | |
| assert x_vals.shape[0] == X.shape[1], \ | |
| "length of x must match number of columns in X" | |
| opts["x"] = x_vals.tolist() | |
| if opts.get("y") is not None: | |
| y_vals = np.asarray(opts["y"]).ravel() | |
| assert y_vals.shape[0] == X.shape[0], \ | |
| "length of y must match number of row in X" | |
| opts["y"] = y_vals.tolist() |
|
|
||
| if opts.get("y") is not None: | ||
| assert len(opts["y"]) == X.shape[0], \ | ||
| "length of y must match number of row in X" |
There was a problem hiding this comment.
The assertion message for y has a grammar issue: "number of row in X" should be "number of rows in X" (and ideally match the phrasing used for rownames). This will be surfaced to users on invalid inputs, so it’s worth correcting.
| "length of y must match number of row in X" | |
| "length of y must match number of rows in X" |
|
A few issues to address before this can be merged:
|
Summary:
This PR adds support for explicit x-and y-axis values in visdom.heatmap(), allowing users to provide
properly Scaled axes instead of replying on inferred pixel indices.
Changes:
*Added optional opts["x"] for explicit x-axis values
*Added optional opts["y"] for explicit y-axis values
*validated axis length against the input heatmap shape
*preserved backward compatibility with existing columnnames / rownames
Motivation:
Currently, visdom.heatmap() always infers axis values from pixel indices, which makes it difficult to interpret heatmaps generated from non-integer or non-uniform Grids.
This change enables users to pass meaningful axis values directly, aligning visdom behavior with common
plotting tools like plotly and matplotlib.
Related issue:
Closes #937
Checklist:
*Change is backward compatible
*No API breakage
*Minimal and scoped to heatmap()
*CI passes input Shapes
*Validates input Shapes
Summary by Sourcery
Allow visdom heatmap plots to accept explicit x and y axis values while preserving existing behavior.
New Features:
Enhancements: