-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add AnalogTrajectory and FreqMap #7437
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
Conversation
In order to keep using unit-style input, I have hacked the _resolve_parameters_ function a little bit. Assume all inputs should either be tunit value or sympy.Symbol. (I.e. no sympy expression is allowed). I have to add this since we don't have SymbolFunc or ExprFunc in cirq world.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7437 +/- ##
==========================================
- Coverage 98.70% 98.69% -0.01%
==========================================
Files 1114 1119 +5
Lines 98301 98438 +137
==========================================
+ Hits 97024 97156 +132
- Misses 1277 1282 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
LGTM! Is this a stage where we should start thinking about hiding the qubit frequency itself (and only refer to shifts from some hidden value), or do we want to keep it visible for now?
cirq-google/cirq_google/experimental/analog_experiments/__init__.py
Outdated
Show resolved
Hide resolved
"""Folder for Running Analog experiments.""" | ||
|
||
from cirq_google.experimental.analog_experiments.analog_trajectory_util import ( | ||
FreqMap as FreqMap, |
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.
Should this be called FrequencyMap to avoid abbreviations.
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.
done
cirq-google/cirq_google/experimental/analog_experiments/analog_trajectory_util.py
Outdated
Show resolved
Hide resolved
cirq-google/cirq_google/experimental/analog_experiments/analog_trajectory_util_test.py
Outdated
Show resolved
Hide resolved
|
||
import cirq | ||
from cirq_google.experimental.analog_experiments import symbol_util as su |
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.
I am not sure about the dependency graph here. I don't think we want to import experimental from a non-experimental file.
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.
ParamResolver is under the cirq/study
so I move this util file into cirq_google/study
. Is that good?
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.
SGTM
Add FreqMap and AnalogTrajectory into cirq_google.experiments.
In this PR, two major differences compared with Trond did before.
AnalogTrajectory
initialized with classmethod so that the class member is always full_trajectoryFreqMap
so that we can provide the symbol into it and plot it with resolver likepulse_plot
New usage example