-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Kibble-Zurek Mechanism with Noise Mitigation #31
base: main
Are you sure you want to change the base?
Conversation
…d plot, and disable the function triger when changing inputs on couplings
… annealing_time parameter (bug in dwave-sampler?).
… calculation for kink_density plot (still need fixing)
…=1 and kink density ~ 0.04
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.
Some high-level comments (still have to review most of the code):
- The logo and the tab-buttons aren't aligned with the edge and jump around quite a bit when resizing the window. It'd look better if they where at a constant distance from the edges, I think.
- When running the ZNE simulation, curve fitting seems to happen after a couple of runs with different coupling strengths. This doesn't seem to work as it should. If the fit fails, there's a popup and everything is temporarily gone. If the fit finishes and a curve appears, the scaling seems to be off. It also disappears (being replaced with the sample data points) if switching view.
- The original demo has no default value for QPU, causing an error (instead of a user-friendly warning) if a run is started without a chosen QPU.
demo_configs.py
Outdated
USE_CLASSICAL = True | ||
J_BASELINE = -1.8 |
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.
Comments on what these do?
Co-authored-by: Theodor Isacsson <[email protected]>
…-zurek into feature/noise-mitigation
The break point jumping behaviour is standard practice for web development that Dash has built in with their Container component. But I agree that the sides should be lined up so I'll remove the Container here. |
Yeah I agree, I wonder if @JoelPasvolsky has a suggestion on which would make a good default? |
I'd suggest following Ocean, just using the default that would be selected by |
@thisac I think I fixed this bug but let me know if you're still seeing it |
@JoelPasvolsky @jackraymond @thisac I've made quite a few updates but I believe we're ready for another round of reviews. |
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.
This is a vast improvement, @k8culver, wow.
Just from playing with it for a bit here are some comments and questions:
- It's unclear to me how the ZNE panel's lower graph can have a predicted line for several values of J: the kink density is dependent to some extent on J so each plotted coupling strength would have a slightly different slope, yet the plot has a single predicted band. I think that might have to be removed, no?
- Panel names: perhaps "Kibble-Zurek Simulation" and "Zero-Noise Extrapolation"?
- Plot names: currently just describing axes and for the lower one using two different names (quench and anneal). Can these be more descriptive of what they are doing? The top one is extrapolating the zero-noise density and the bottom one is displaying the measured and extrapolated kink densities
- Left panel introduction: currently explains the science but might be more useful to tell the user how to run the demo (it's more complicated now and needs a few steps to get useful results) and put the science explanation in the readme.
- Needs rebasing (currently does not have the adv7 cached files)
- The button to turn off tool tips seems to have gone missing. That is useful for people giving this demo to an audience.
- I'm getting debug prints in my terminal
I'll also go over the readme. If I have time I'll look over the code changes but I might not and have to leave that to T and J.
|
||
<img src='assets/ZNE_fig2.png' alt='Experimental results' width='400'/> | ||
|
||
|
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.
Consider adding the explanation currently in the left-panel intro ("Owing to thermal noise, ... to larger quench durations") here and using that space to tell users how to run an experiment in three-ish steps
|
@JoelPasvolsky I think I've addressed everything. I'm not sure how to proceed with 1) and 4) based on @jackraymond's feedback
Will defer to @jackraymond on this as he understands the problem better.
Updated here: d18c318
Updated to "Extrapolating Zero-Noise Density" and "Measured and Extrapolated Kink Densities", let me know if those make sense. Updated here: d18c318
I am planning on doing a follow up PR to have the autoplay option. Would this solve this problem?
These changes will show once merged. If you would like this for testing I can pull the changes in.
I removed it as it cluttered the UI and distracted from the problem settings, I've added the functionality back but as a setting in the
These can be hidden by setting |
Thanks @AndyZzzZzzZzz for your work on this feature!
I can't remember if there were additional updates we wanted to add, all I've changed is cleaning up the code a bit and fixing the tests.
@jackraymond is going to help add tests for Andy's work.
I will follow this PR with a PR updating the code structure, adding more thorough doc strings, etc, for now this is mostly just Andy's updates.