-
Notifications
You must be signed in to change notification settings - Fork 127
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 fit model to CurveAnalysis #726
Add fit model to CurveAnalysis #726
Conversation
@nkanazawa1989 I need more documentation in order to review this PR. Required are both inline comments inside methods, to describe the inner variables, their data structures and the manipulations that they're going through; and more detailed doc strings, with more explanation and examples. Specifically I started with the new file Trying to figure out what's the role of the When strings are involved - in signatures and fit models - it's important to specify the expected syntax of the strings. Namely, how to write a string that implements to a certain signature. |
79193a8
to
8be5f0c
Compare
I added more documentation. Basically this PR combines parameter mapping code distributed across multiple functions/files into a single place as These are the logic currently implementing the curve analysis: and parameter mapping is done in I also removed Composite to avoid confusion. For the variadic arguments you can refer to the API docs of scipy curve fit. There is no detailed docs there too. |
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 think we should use this PR as an opportunity to refactor how fixed_parameters are defined. Overall the class FitModel
and its sub-classes seems like a nice idea.
Co-authored-by: Daniel J. Egger <[email protected]>
Co-authored-by: Daniel J. Egger <[email protected]>
….com:nkanazawa1989/qiskit-experiments into upgrade/cleanup-curve-analysis-add-fit-model
releasenotes/notes/add-fit-model-and-remove-curve_fitter-arg-4a0effb5f9b88ba9.yaml
Outdated
Show resolved
Hide resolved
upgrade: | ||
- | | ||
Analysis option `curve_fitter` of the :class:`CurveAnalysis` has be removed | ||
because of the serialization problem. To use custom curve fitting library, |
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.
Which serialization problem?
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.
You know, the callable is not serializable.
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.
because of the serialization problem. To use custom curve fitting library, | |
because callable lambda functions are not serializable. To use a custom curve fitting library, |
?
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 not quite true. Please check the update
cd19b81
releasenotes/notes/add-fit-model-and-remove-curve_fitter-arg-4a0effb5f9b88ba9.yaml
Outdated
Show resolved
Hide resolved
:meth:`CurveAnalysis.curve_fit` has been added to the curve analysis and | ||
its subclasses. Now you can directly access to the core fitting code | ||
with bare numpy arrays representing data to be fit. | ||
This may help debugging of new fit function. |
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 may help debugging of new fit function. | |
This may help debugging of new fit functions. |
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.
Please add a reference to an example that demonstrates how to use the new feature for debugging fit functions.
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.
There is no example, but user can directly input ndarrays without constructing ExperimentData
container. Since there is no user guide how to construct experiment data, it is very tough to test fit functions in the curve analysis without reading the code of experiment data which is super long. I can remove
This may help debugging of new fit functions.
If I need to write mode docs just for it.
…ade/cleanup-curve-analysis-add-fit-model
Co-authored-by: Yael Ben-Haim <[email protected]>
95855cb
to
17560c3
Compare
08f7cbc
to
a7f5375
Compare
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'm still a bit confused by this PR. See my questions that try to clarify. It seems like there is a tight coupling between SeriesDef
and FitModel
. This is fine but the code does not make it very explicit. I wonder if both classes are needed. Or if SeriesDef
should be called FitModelConfig
and FitModel
should be initialized form the config, i.e. FitModel.from_config(config: FitModelConfig)
?
|
||
y = np.zeros(x.size, dtype=float) | ||
for i, (func, sig) in enumerate(zip(self._fit_functions, self._signatures)): | ||
inds = self.data_allocation == i |
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.
If you are so worried about performance, wouldn't a dict be better to have O(1) lookup? E.g.
inds = self.data_allocation[i]
This should replace O(n) for the ==
with O(1) for the dict look-up.
upgrade: | ||
- | | ||
Analysis option `curve_fitter` of the :class:`CurveAnalysis` has be removed | ||
because of the serialization problem. To use custom curve fitting library, |
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.
because of the serialization problem. To use custom curve fitting library, | |
because callable lambda functions are not serializable. To use a custom curve fitting library, |
?
|
||
function = SingleFitFunction( | ||
fit_functions=[child_function], | ||
signatures=[["par0", "par1"]], |
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.
What happens if I give signatures=[["dummy1", "dummy2"]]
i.e. the names do not match? I though that part of this whole exercise is so that we do not need to worry about the signature of the fit_functions
. Looking at SeriesDef
I see that SeriesDef
figures this out. Would it be more natural to do this inspection in FitModel
instead of SeriesDef
or something like FitModel.from_series_def
?
Co-authored-by: Daniel J. Egger <[email protected]>
593c393
to
ad70b0d
Compare
Co-authored-by: Daniel J. Egger <[email protected]>
8c71a8a
to
50d984a
Compare
Thanks @eggerdj . Indeed I'm thinking of (EDIT) |
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 pretty much fine with this PR my comments are only minor. @yaelbh can you take a second look?
Co-authored-by: Daniel J. Egger <[email protected]>
ddedefa
to
08c65fc
Compare
The It has the There's a lot of examples here https://lmfit.github.io/lmfit-py/examples/index.html |
Thanks @zlatko-minev I did detailed investigation for This is quite important in our experiment library (e.g. Ramsey XY, fine amplitude, HEAT, CR Hamiltonian tomography, IRB, etc...) but the model interface for this is not supported in the library.
For example, in the fine amp example, from lmfit import Model, CompositeModel
import numpy as np
import matplotlib.pyplot as plt
def spam_cal(x, amp, base):
return base + 0.5 * amp * (2 * x - 1)
def ping_pong(x, amp, d_theta, phase_offset, base, angle_per_gate):
return 0.5 * amp * np.cos((d_theta + angle_per_gate) * x - phase_offset) + base
# x values for each model
x1 = np.array([0, 1])
x2 = np.arange(0, 15)
# some parameters
amp = 0.9
d_theta = 0.01
phase_offset = np.pi/2
base = 0.48
angle_per_gate = np.pi
# simulated y data
data1 = spam_cal(x1, amp, base)
data2 = ping_pong(x2, amp, d_theta, phase_offset, base, angle_per_gate)
# standalone model
# How to combine them? Currently lmfit doesn't provide interface to combine.
# Note that this is not CompositeModel where it takes the same x-value and different parameters.
m1 = Model(spam_cal)
m2 = Model(ping_pong)
# so we need to dynamically generate function like below
def composite_func(x, amp, base, d_theta, phase_offset, angle_per_gate, separator):
# giving these as kwargs is more efficient but lmfit Model parses signature for the model
# **kwargs doesn't provide any explicit parameter name
params = {
"amp": amp,
"base": base,
"d_theta": d_theta,
"phase_offset": phase_offset,
"angle_per_gate": angle_per_gate,
}
y = []
for mi, xi in zip((m1, m2), np.split(x, separator)):
y.append(mi.eval(x=xi, **{p: params[p] for p in mi.param_names}))
return np.concatenate(y, 0)
# this is what we need
composite_model = Model(composite_func) Considering this, I concluded we cannot employ lmfit library until it supports multi-objective model. However, I still think this is great library because it can provide more statistical information on the fitting, and it can hide uncertainties package (unumpy functions) from the fit model. I'll continue to watch this library. |
That's interesting. I definitely used it for Ramsey XY fitting and multiple data sets before. You just concatenate the data from the different sets into a large one dimensional array, my experience that usually works pretty well. |
Yes, their fitter ( Another approach would be merging this PR as-is, and write |
Perhaps concatenated array that you mention is a technique to use
In this case we need to dynamically generate |
Replaced with #806 with different implementation with LMFIT. |
Summary
This PR is the first step to introduce group in #715. The purpose of the PR is to add a fit model class to
CurveAnalysis
and to removecurve_fitter
from analysis option.Details and comments
According to the review comments from @yaelbh and @eggerdj, the complexity of the
CurveAnalysis
is likely an issue to further extend the class. This PR introduces new classFitModel
and its subclassSingleFitFunction
andCompositeFitFunction
that user can also check to grasp the model without reading the code. For example:Now you can recall
FineAmplitudeAnalysis
does SPAM correction with extra experiment, and also some parameters are fixed during the fitting.From software point of view, the complexity of multi objective optimization is all offloaded to the fit model, and the rest of code doesn't need to take care how the fit model is implemented. If there is only single curve in the analysis, it implicitly uses
SingleFitFunction
which implements lightweight logic to compute y data.