Replace __fixed_parameters__ with analysis option
#734
Conversation
| fit_params = cls._fit_params() | ||
| options.p0 = {par_name: None for par_name in fit_params} | ||
| options.bounds = {par_name: None for par_name in fit_params} | ||
| options.p0 = {} |
There was a problem hiding this comment.
Hint for review. This cannot be initialized with parameter names, because fixed params are now instance attribute that user can modify after the instance is created. Note that now we have .parameters property, thus user can know what parameters live in the fit model.
nkanazawa1989
force-pushed
the
upgrade/make_fixed_params_option
branch
3 times, most recently
from
831b900 to
e6d84d7
Compare
…s attribute __fixed_parameters__
nkanazawa1989
force-pushed
the
upgrade/make_fixed_params_option
branch
from
e6d84d7 to
4224bd5
Compare
| assigned_params = self.options.fixed_parameters | ||
|
|
||
| if assigned_params: | ||
| # Check if all parameters are assigned. | ||
| if any(v is None for v in assigned_params.values()): | ||
| raise AnalysisError( | ||
| f"Unassigned fixed-value parameters for the fit " | ||
| f"function {self.__class__.__name__}." | ||
| f"All values of fixed-parameters, i.e. {self.__fixed_parameters__}, " | ||
| f"All values of fixed-parameters, i.e. {assigned_params}, " | ||
| "must be provided by the analysis options to run this analysis." | ||
| ) |
There was a problem hiding this comment.
Can be a bit more concise and amend the code after this block too?
if any(v is None for v in self.options.fixed_parameters.values()):
raise AnalysisError(f"All values of {self.options.fixed_parameters} must be provided.")then the line below can become if it is not too long:
dict_def["fit_func"] = functools.partial(series_def.fit_func, **self.options.fixed_parameters)There was a problem hiding this comment.
Yes this is good suggestion but anyways this code block will be immediately removed by #726 so I just leave this as is
qiskit_experiments/library/characterization/analysis/fine_drag_analysis.py
Show resolved
Hide resolved
| @@ -253,9 +253,10 @@ def __init__(self, qubit: int, backend: Optional[Backend] = None): | |||
| super().__init__([qubit], XGate(), backend=backend) | |||
| # Set default analysis options | |||
| self.analysis.set_options( | |||
| angle_per_gate=np.pi, | |||
| phase_offset=np.pi / 2, | |||
| amp=1, | |||
There was a problem hiding this comment.
We no longer need to fix amp=1 right? Was this missed in a previous PR?
There was a problem hiding this comment.
Right, indeed this is not fixed. amp is in the default option of this analysis to give some initial value (this is bit strange) but not listed in the fixed parameters.
| fixed_parameters={ | ||
| "angle_per_gate": 0.0, | ||
| "phase_offset": np.pi / 2, | ||
| "amp": 1.0, |
There was a problem hiding this comment.
Is amp=1.0 still needed? If so we can leave as is. This is also not really related to this PR.
There was a problem hiding this comment.
Exactly. I just copied current implementation. For DRAG one I think amp is needed otherwise you need to add spam cal experiment like fine amp.
chriseclectic
added
the
Changelog: API Change
…#734) Add fixed parameters to the curve analysis fit option and remove class attribute __fixed_parameters__
Summary
This PR adds new
fixed_parametersoption to theCurveAnalysisand remove corresponding class attribute, as proposed in #726 by @eggerdj. This change allows developers/end-users to flexibly set analysis class without defining a new class just to fix parameters in the model.Details and comments
The most of logic/documentation will be replaced by #726 thus added code here is just temporary. No careful review or better/smarter logic is needed as long as all unittests pass. The important point of this PR is how API changes.