Skip to content
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

Fix channel value bug and revamp pulse sim testing #811

Merged
merged 73 commits into from
Jul 6, 2020

Conversation

DanPuzzuoli
Copy link
Contributor

@DanPuzzuoli DanPuzzuoli commented Jun 26, 2020

Summary

Closes #803 , and restructures the integration tests of the pulse simulator to attempt to be more systematic.

This PR builds on PR #784 , so review should wait for that to be merged.

Details and comments

Changes

  • The main functional change is correcting the error pointed out in issue Channel value evaluation in pulse simulator is inconsistent with the pulse spec #803: the chan_value function in numeric_integrator.cpp has been modified to have both complex exponentials have positive sign, and return only the real part of the computed value.
  • Minor changes to pulse_controller: added 'method' as a valid de option, and added the ability to set initial state.
  • The test_pulse_simulator.py test file has been revamped; it was necessary to change all tests after fixing chan_value, and this was a good opportunity to make things more rigorous.
    • This file has been changed heavily, though in spirit all of the same tests are being done. The main addition is the file pulse_sim_independent.py, which contains simulation routines for some "standard" models, and besides using the DE_Methods.py file, is totally independent of the pulse simulator. Most of the tests in test_pulse_simulator.py now contain comparison of the results of the de solving with these independent simulations.
    • The simulations in pulse_sim_independent.py take very little code and are meant to be fairly simple, and as such it is much easier to verify their correctness as compared to the full process of the pulse simulator itself.
    • Some of the tests also contain comparisons to approximate analytic solutions (obtained using the rotating wave approximation). In these comparisons, the tolerance for acceptance is lower than in the comparison to independent simulations as the quality of the approximation is not quantified (as compared to a DE solver).

Notes

  • Despite it being a seemingly very small change, the fix to chan_value has resulted in a notable slow down to the pulse simulator. I'm not totally sure why, but perhaps the corrected DEs are more challenging to solve.

Status

  • Still need to clean things up (comments, linting, etc)
  • Done

…folder to just de in anticipation of further additions, added test files for DE_Methods and type_utils
…eferences so that tests pass again, and updated test_type_utils.py to import from the right place
…n tests pass, but the noisy mc solver routines hang
…object apart into model pieces, ode_options, etc
… solvers have different expected defaults even of the same name, may need to move default handling directly into each DE_Method
…ions, and contains an instance of DE_Options
@chriseclectic chriseclectic removed the on hold Can not fix yet label Jul 1, 2020
@DanPuzzuoli DanPuzzuoli changed the title [WIP] Fix channel value bug and revamp pulse sim testing Fix channel value bug and revamp pulse sim testing Jul 1, 2020
Copy link
Contributor

@vvilpas vvilpas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been trying to run this tutorial 8_pulse_simulator_duffing_model and the solver raises some warnings ("UserWarning: qiskit_zvode: Excess work done on this call. (Perhaps wrong MF.)") and then raises an exception: (ODE method exited with status: -1). Do I have to change something in the tutorial?

@DanPuzzuoli
Copy link
Contributor Author

DanPuzzuoli commented Jul 3, 2020

Yes good catch - I had noticed this but it slipped my mind. I was able to get the notebook to work by using the DE method scipy-RK45 instead of zvode (this is one of the new methods available from the DE_Methods PR). This can be done through backend_options:

backend_options = {'solver_options' : {'method' : 'scipy-RK45'}}
sim_result = backend_sim.run(cr_rabi_qobj, two_qubit_model, backend_options).result()

I'm not totally sure what is going on with zvode and why it raises this error - it has been suggested that this may be due to discontinuities in the RHS function (as the pulses are piecewise constant), but I'm not sure I'm sold on this as it seems to work fine in most other instances (e.g. the simulations earlier in the notebook also contain such discontinuities). My guess is zvode may just be hitting some lower limit on step size or an upper limit on number of steps (perhaps there is some option we can change?), as the cross-resonance simulations may have some fast oscillating terms in the solutions (or perhaps it is a combination of discontinuities and fast oscillations that are causing problems?).

One simple option for now is to just change the default solver to scipy-RK45. This is rudimentary, but comparing speeds of running all of the pulse simulator tests using scipy-RK45 on this PR seems to consistently take ~24-25 seconds, whereas with zvode they take ~21-22 seconds. So, zvode is faster but not by much (though admittedly this gap may appear larger after your RHS function improvements are included). Thoughts?

@DanPuzzuoli
Copy link
Contributor Author

DanPuzzuoli commented Jul 6, 2020

Looking at the zvode documentation, the -1 means excess work done on this call. (Perhaps wrong MF.) is described as:

C          -1  means an excessive amount of work (more than MXSTEP
C              steps) was done on this call, before completing the
C              requested task, but the integration was otherwise
C              successful as far as T.  (MXSTEP is an optional input
C              and is normally 500.)  To continue, the user may
C              simply reset ISTATE to a value .gt. 1 and call again.
C              (The excess work step counter will be reset to 0.)
C              In addition, the user may increase MXSTEP to avoid
C              this error return.  (See optional input below.)

I'm not sure but MXSTEP may be the equivalent of nsteps in the scipy interface. I

Edit:

I just tried increasing nsteps to 10**10 (just a huge number as np.inf wasn't a valid input) and the notebook is now running properly. Seems like we should increase this value but I don't know what a reasonable number is.

Further Edit:

The cross-resonance simulation may be a good place to do simulation speed tests. After eliminating the zvode issue, I ran the last simulation cell using both zvode and scipy-RK45 (edit: in my local copy of the tutorial I have reduced the number of cr_drive_amps to 8 to reduce simulation time):

  • zvode takes ~80 seconds, and
  • scipy-RK45 takes ~40 seconds,

with no discernible difference between the outputs. Also worth noting is that the speed of zvode seems to not depend on the order argument. I.e. the discrepancy above could be due to RK45 having a much lower order, but even setting order=4 for the zvode solver results in simulations taking ~80s. Despite the tests being a bit slower with scipy-RK45 it seems to have a huge advantage here.

This last speed discrepancy makes me want to switch for now to scipy-RK45 as the default solver.

@vvilpas vvilpas mentioned this pull request Jul 6, 2020
vvilpas
vvilpas previously approved these changes Jul 6, 2020
Copy link
Contributor

@vvilpas vvilpas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@DanPuzzuoli
Copy link
Contributor Author

Just increased value of nsteps to 10**6, and the tutorial runs. I ended up ultimately deciding to stick with zvode as the simulations using either zvode or scipy-RK45 seem to take about the same amount of time in the tutorial and zvode is a bit faster in testing. (This is odd given the observed discrepancies in the previous comment. These latest comparisons are the result of resetting the tutorial to its current state. Not sure what has caused the change, but resolving which solver to use should be part of more systematic speed tests anyway.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Bugfix Include in the Fixed section of the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Channel value evaluation in pulse simulator is inconsistent with the pulse spec
3 participants