-
Notifications
You must be signed in to change notification settings - Fork 7
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 fitting to calibration routines #25
Conversation
@andrea-pasquale thanks, the mechanism looks good to me, please remember to update the sphinx docs accordingly explaining how to implement a complete action. |
Thanks for the feedback @scarrazza.
About the plotting decorators. what did you have in mind? |
My opinion is the following:
|
@scarrazza Regarding scans with two variables, how will you be able to fit every row? This comes back to the point I had raised about being able to convert the flat array into a matrix. |
I've implemented the live-fitting for the resonator spectroscopy.
This PR have been tested on qpu1q (runcard example1q.yml) and qpu5q (runcard example5q.yml). Everything is now hardcoded for the resonator spectroscopy, if you are happy with this, I'll start to reorganize the code. |
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.
Thanks for implementing this. I like the idea of adding the live fitting line to the live plotting, but let's also hear David's and Maxime's opinion as the main users of this.
If fitting is fast for all actions, I do not see any issue with putting it on all live plots and reports. On the other hand, if fitting is causing any bottleneck, we should consider having a way to switch it on or off (perhaps through an option in qq or qq-live) in case someone wants to run a scan without fitting.
src/qcvv/fitting/utils.py
Outdated
def get_values(df, quantity, unit): | ||
return df[quantity].pint.to(unit).pint.magnitude |
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.
In #26 which is already merged to add_routines
I added this as a method to the Dataset
object, because it is also useful for plotting. So you can probably remove this from here and just do data.get_values
when you use it in the fitting methods.
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 also like the idea of the live fitting. As @stavros11 pointed out, if there is no bottleneck due to the live fitting, I really like the idea. If it is not the case, I agree in the idea of having a way to switch it on and off.
Thanks @andrea-pasquale for implementing this functionality.
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.
In #26 which is already merged to
add_routines
I added this as a method to theDataset
object, because it is also useful for plotting. So you can probably remove this from here and just dodata.get_values
when you use it in the fitting methods.
Thanks, I forgot to remove it here.
src/qcvv/decorators.py
Outdated
f.prepare = prepare_path | ||
f.final_action = save | ||
return f | ||
|
||
|
||
def fit(f): |
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.
At this stage, I am not really sure if the store
and fit
decorators are the best way to go, especially if we end up using them in all calibration methods. To me, it seems easier to have store
and fit
enabled for all calibrations, for example through the action builder, without having to explicitly use @store
and @fit
in every method. I may be missing something so let's see what others think.
In #45 I implemented a plot decorator but this is different as it can be used to map multiple calibration methods to the same plotting function. This is relevant for the calibrations added by Maxime and David. Perhaps when you implement fit for more calibrations, something similar may be useful (eg. @fit()
with arguments). But at least for store, it seems that it is the same for all calibration methods and it is also essential for all other features (fitting, report, live plotting), so we could consider hard-coding it to the ActionBuilder and removing the decorator.
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.
Also agree with @stavros11. At the end, we are going to store and fit all the data obtained from the calibration methods. To me the @store and @fit decorators should be enabled in all calibrations by default.
Regarding #45, this functionality is also important, because until now I had to implement a method for plotting for each calibration routine even if they were more or less the same (f.e when only units changed, I had to code a new function).
Thanks @stavros11
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.
Thanks for the feedback. I also agree that at this point especially store should not be a decorator, given that the (live)plotting is reading the data directly from file. Regarding the fitting I think that we can follow a similar approach for the plotting. We can always perform the fitting and save the results in the report. Then we can have a decorator in order to perform the live-fitting which should be optional.
src/qcvv/web/app.py
Outdated
if len(data) > 2: | ||
params, fit = resonator_spectroscopy_fit(folder, format, nqubits) | ||
return getattr(plots.resonator_spectroscopy, method)(data, params, fit) |
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 could just be:
if len(data) > 2: | |
params, fit = resonator_spectroscopy_fit(folder, format, nqubits) | |
return getattr(plots.resonator_spectroscopy, method)(data, params, fit) | |
if len(data) > 2: | |
params, fit = resonator_spectroscopy_fit(folder, format, nqubits) | |
else: | |
params, fit = None, None | |
return getattr(plots.resonator_spectroscopy, method)(data, params, fit) |
and have a single return.
Also, keep in mind that this method is out-dated in this branch, as we have been updating it with Maxime as he was adding more calibrations in his branches. I would say the latest stable version that works on the actual experiments is in maxime/routines
branch and I simplified it a bit more in #45 where I removed plots.yml. The important thing is that when you generalize this to arbitrary actions, you will need a getattr
-like approach to call the proper fitting method for your action.
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.
Thanks.
src/qcvv/web/app.py
Outdated
with open(f"{folder}/platform.yml", "r") as f: | ||
nqubits = yaml.safe_load(f)["nqubits"] |
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 method is called automatically every second by dash to update the figures, so we should optimize it as much as possible. For example here we could pass nqubits (assuming its a constant) through the url, similarly to routine, format, etc., which is rendered once during the first page load, to avoid reading from the platform.yml from disk every second.
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 completely agree, this was just a temporary solution to have things working.
src/qcvv/web/app.py
Outdated
with open(f"{folder}/platform.yml", "r") as f: | ||
nqubits = yaml.safe_load(f)["nqubits"] | ||
if len(data) > 2: | ||
params, fit = resonator_spectroscopy_fit(folder, format, nqubits) |
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.
Similarly to the comment above, here you are fitting the data in every live plot update. Would it be easier to do the fit within the action, dump the results on disk and just load them here for plotting (similar to what we do with data)?
I think that as it is, given that the live plot is typically updated more frequently than the actual data, this will perform the same fit multiple times before new data are available.
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 that this could be a good idea, given that some of the calibrations will also require fitting inside the routine itself and not as a post-processing step.
I think that as it is, given that the live plot is typically updated more frequently than the actual data, this will perform the same fit multiple times before new data are available.
This is true. If we decide to include the fitting inside the calibration we should then be able to call the fitting using a parameter similar to points
to decide when we should perform the fitting and save it to file.
|
||
class resonator_spectroscopy_attenuation: | ||
|
||
class resonator_spectroscopy: |
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 also out-dated. In maxime/routines
we simplified the plots to functions instead of classes, as the same plot function can be re-used in multiple routines.
Thanks for the review and the comments @DavidSarlle @stavros11. |
okay, sounds reasonable. |
Now this PR works with the I've implemented the following changes:
Regarding 2 the fitting is now performed directly inside the routine. As @stavros11 pointed out, in order to avoid unnecessary calls to the fitting, the fitting is performed the same time that we store to file using the parameter |
I've also added the fitting on the qubit spectroscopy. |
@stavros11 can we merge this now? |
In this PR I've implemented a first prototype for the post-processing of the data generated by the calibration routines.
It will be sufficient to add the decorator
@fit
to a calibration routine to do the following:In order to do this I've created a module called
fitting
which contains two different files:methods.py
which contains routine-specific fitting methods of the form<routine_name>_fit
update.py
which contains routine-specific methods for updating the platform runcard of the form<routine_name>_fit
The
ActionBuilder
will take care of associating each calibration routine with the corresponding methods for fitting and updating the platform runcard. For now, the fitting methods are the same that are used in the branch alvaro/latest of qibolab.This branch is working with this branch in qibolab, since I am using the latest calibration routines and the new names for the platform runcard which are not been yet pushed to main.
@scarrazza @stavros11 let me know if you like this approach.
It should not be difficult to use the same approach for generating plots.