Add orbital entanglement diagram widget#2974
Conversation
source/widgets/js/render_svg.mjs
Outdated
| // ---- Circuit (imperative DOM via qviz) ---- | ||
| case "Circuit": { | ||
| // qviz.draw() needs a real DOM. Use jsdom. | ||
| const { JSDOM } = await import("jsdom"); |
There was a problem hiding this comment.
This feature assume a) Node is on the users PATH (and at least it gives an error message if not), and b) That jsdom is available as a package to import in node (which it isn't by default, the user would need to install it into the global (or local) node environment. This seems fragile and likely to fail unless users have followed some specific setup instructions first. (We also don't want to bundle JSDOM into our package as it's huge).
Not sure what to do about this, but worth thinking about.
There was a problem hiding this comment.
Agree, an initial version had JSDOM as dependency but I removed it for the reason you stated. I dont think I can get rid of the Node requirement if I want to allow rendering without actually displaying the widget. But I can shim/mock the few parts of JSDOM we need.
There was a problem hiding this comment.
I added the shim, but its 500 lines of extra stuff, thoughts?
| "monaco-editor": "^0.44.0", | ||
| "openai": "^4.83.0", | ||
| "preact": "^10.20.0", | ||
| "preact-render-to-string": "^6.6.6", |
source/widgets/js/render_svg.mjs
Outdated
| } | ||
|
|
||
| // ---- Circuit (imperative DOM via qviz) ---- | ||
| case "Circuit": { |
There was a problem hiding this comment.
Did you try this at all? Thinking through how this works, you wouldn't get what's displayed on screen (unless you've not interacted with it). You'll just get the default new rendering with a circuit, which now has all blocks/calls/operations collapsed. If the user has drilled into operations to expand them and show the gates etc., that's not going to show up here, as it's re-rendering the circuit from a clean state, not the current UX state.
There was a problem hiding this comment.
Yes that was the intended behavior (for now).
The point of these functions was not so save the user-altered state of the widget into an SVG. (Though that would be even better!)
The idea was to be able to make the widget class, call the SVG dump and never need to look at it to extract an SVG to later view/use.
Will see if I can get it to work using the actual state of the widget.
There was a problem hiding this comment.
I have now added functionality to use the actual state of the widgets but still allowing a different path to render without the widget being displayed to the user.
| /** | ||
| * Detect whether the host background is dark or light by sampling the | ||
| * computed background-color of the nearest ancestor with one. | ||
| * Returns a high-contrast colour for selection outlines. |
There was a problem hiding this comment.
Why not use CSS values to match the current theme like the other widgets?
| } = props; | ||
|
|
||
| // --- theme-resolved colours --- | ||
| const FONT_FAMILY = '"Segoe UI", Roboto, Helvetica, Arial, sans-serif'; |
There was a problem hiding this comment.
Again, we should use CSS for appearance properties like font selection and theme colors. Baking it into the code is hard to manage and get consistent behavior across widgets.
| ? labelsProp | ||
| : Array.from({ length: n }, (_, i) => String(i)); | ||
|
|
||
| // --- colour scales --- |
There was a problem hiding this comment.
Nit: the spelling of 'color' or 'colour' is inconsistent every few lines (e.g. line 287, 289, 303, etc.). Being most code authors are in the U.S., we should probably pick the U.S. spelling and stick with it.
| el.ownerDocument.head.appendChild(forceStyle); | ||
| } | ||
|
|
||
| // Belt-and-suspenders: also set the background inline on the nearest |
There was a problem hiding this comment.
What does "Belt-and-suspenders" mean? Also, is the class you are looking for below consistent across notebooks in VS Code, notebooks in the browser, and notebooks in Google Colab? Again, prefer the detection and theme selection we already do in other widgets via CSS for consistency in behavior across widgets.
source/widgets/js/index.tsx
Outdated
| clone.querySelectorAll(stripSelector).forEach((n) => n.remove()); | ||
| } | ||
| // Remove interactive-only elements common across widgets | ||
| clone |
There was a problem hiding this comment.
I think this is an interesting approach (cloning and serializing the SVG), but this implementation seems hard to maintain and fragile. Below you are baking knowledge of the internals of various widgets into this one central location outside of the widgets. This would mean if we ever touch those widgets (e.g. rename the .menu-icon class or #menu id or some of the other classes or styles hard-coded below, we'd need to remember to come here and change it also. That would be easy to miss and introduce bugs we wouldn't find for a while (being automated testing for this is hard)
I'm also not a big fan of automatically watching for changes and cloning the svg on mutation, being the SVG can be really huge (esp. for circuits, and likely for chord diagrams), and the vast majority of time people aren't going to use the 'save the svg feature'.
Is there a way to push the 'cleaning' of the SVG/DOM to the owning widget (so it's easy to see when changing the widget itself), and only clone & serialize the SVG when the user actually wants to save the SVG?
There was a problem hiding this comment.
Fully agree, this is all an artifact of the idea that I outlined below with your question about the second svg export route.
source/widgets/js/svgDomShim.mjs
Outdated
| } | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
This is a lot of very subtle code (the DOM has a lot of nuance) to turn around in a few hours, and will take careful review. Did you personally write this? Or is it taken from another library or AI generated?
There was a problem hiding this comment.
This was AI generated to meet the needs for now. I consider this still in the ideation stage to see what could be the best way of getting the result we are looking for.
I will draft the PR to make this a bit clearer.
| import circuitCss from "../../npm/qsharp/ux/qsharp-circuit.css"; | ||
|
|
||
| const input = readFileSync(0, "utf-8"); // stdin | ||
| const { component, props } = JSON.parse(input); |
There was a problem hiding this comment.
If you're cloning and serializing the SVG above now, why do you need to shell out to node and render the component via the prop at all? What's the use-case for this file now?
There was a problem hiding this comment.
The use case I have in mind is not having a browser/notebook/vscode instance at all.
Essentially thinking about someone running qdk-chemistry or any other component on a remote HPC system and wanting to generate a picture to quickly see if a calculation did what it was supposed to.
If I understand correctly, anywidget uses the DOM of whatever the widget is shown in, meaning that if there is not VSCode/jupyterlab/etc, there also is no DOM to render the SVG with.
source/widgets/js/render_svg.mjs
Outdated
| // Add xmlns for standalone SVG and set a reasonable default render size | ||
| html = html.replace( | ||
| /<svg\s+class="histogram"/, | ||
| '<svg xmlns="http://www.w3.org/2000/svg" class="histogram" width="600"', |
There was a problem hiding this comment.
This is just way too fragile. You're looking for specific CSS classes and ids from various widgets to do fixups in multiple places in this code (line 76, 77, 88, 124, 130). This will break as soon as anyone touches those style/class names (which we've done a bit), or adds new styles/classes that this code would also need to handle. I think we need to rethink how to handle this special cases, maybe with something consistent and foolproof across widgets.
| onWheel={isStatic ? undefined : onWheel} | ||
| {...(embedCss | ||
| ? { | ||
| xmlns: "http://www.w3.org/2000/svg", |
Check warning
Code scanning / devskim
An HTTP-based URL without TLS was detected. Warning
| {...(embedCss | ||
| ? { | ||
| xmlns: "http://www.w3.org/2000/svg", | ||
| "xmlns:xlink": "http://www.w3.org/1999/xlink", |
Check warning
Code scanning / devskim
An HTTP-based URL without TLS was detected. Warning
Note: This PR is a lot of AI generated infra to achieve an outcome for testing purposes. While the features work, the way they are done needs some serious work and rethinking.
Adds:
Widget with active space selection (gold):

Widget while hovering individual orbital/entangled node:
