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

Add warning if digital carrier exceeds Nyquist frequency in pulse -> signal conversion #242

Merged
merged 6 commits into from
Jul 14, 2023

Conversation

DanPuzzuoli
Copy link
Collaborator

Summary

Closes #241

A warning is added to the pulse -> signal converter if the digital carrier frequency (resulting from SetFrequency and ShiftFrequency instructions) exceeds the Nyquist frequency set by the sample size dt.

Details and comments

The warning is suppressed if the conversion is called within JAX tracing, as in this case the boolean function np.abs(frequency_shift) > 0.5 / dt cannot be evaluated.

A test has been added, and @nkanazawa1989 I've verified that the warning is raised in your example that motivated this PR.

Copy link
Contributor

@nkanazawa1989 nkanazawa1989 left a comment

Choose a reason for hiding this comment

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

Thanks @DanPuzzuoli this is really helpful improvement. I was thinking shift and set frequency were directly applied to the analog carrier, and had trouble with interpretation of weird simulation results. I think I am not only person suffering this problem :)

The warning is suppressed if the conversion is called within JAX tracing, as in this case the boolean function np.abs(frequency_shift) > 0.5 / dt cannot be evaluated.

This means the warning is also raised at run time even with the JAX backend? Then this PR looks good to me.

@@ -188,14 +198,15 @@ def get_signals(self, schedule: Schedule) -> List[DiscreteSignal]:
if isinstance(inst, ShiftPhase):
phases[chan] += inst.phase

if isinstance(inst, SetPhase):
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this related to the warning?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is not related to the warning: I saw that the SetPhase and ShiftPhase handling were not grouped together, so I moved this block up.

@DanPuzzuoli
Copy link
Collaborator Author

Thanks @DanPuzzuoli this is really helpful improvement. I was thinking shift and set frequency were directly applied to the analog carrier, and had trouble with interpretation of weird simulation results. I think I am not only person suffering this problem :)

The warning is suppressed if the conversion is called within JAX tracing, as in this case the boolean function np.abs(frequency_shift) > 0.5 / dt cannot be evaluated.

This means the warning is also raised at run time even with the JAX backend? Then this PR looks good to me.

The error will not be raised ever if the converter is called within a traced function. For now this probably won't impact any users; it will still be raised when using the DynamicsBackend using JAX as you are in your notebook.

As an aside: This is an unfortunate element of the way JAX works - you can't have warnings/errors that depend on the value of variables that are being traced. Branching logic that depends on the value of a variable will cause an error during tracing, and once compiled, the original code is no longer executed anyway, so no warnings can be raised there.

@DanPuzzuoli DanPuzzuoli merged commit ea941cb into qiskit-community:main Jul 14, 2023
DanPuzzuoli added a commit to DanPuzzuoli/qiskit-dynamics that referenced this pull request Aug 2, 2023
DanPuzzuoli added a commit that referenced this pull request Aug 8, 2023
* adding sympy as an explicit requirement (#233)

* Add warning for JAX versions on import of Dynamics (#232)

* Add probability normalization to DynamicsBackend sampling routine (#239)

* Add warning if digital carrier exceeds Nyquist frequency in pulse -> signal conversion (#242)

* Fix bug with carrier_freq being a JAX tracer if envelope is constant in Signal (#247)

* Remove subsystem_labels option from DynamicsBackend (#248)

* Rename subsystem_dims Dict to subsystem_dims_dict (#250)

* Fix ClassicalRegister counting in DynamicsBackend (#252)

* temporary fix for docs (#253)

* Drop support for backendV2 in from_backend (#249)

* incrementing version number

* cleaning up reno files

---------

Co-authored-by: Kento Ueda <38037695+to24toro@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add warning about sampling rate when using ShiftFrequency and SetFrequency in pulse schedules
3 participants