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

Classifiers #447

Merged
merged 34 commits into from
Aug 21, 2023
Merged

Classifiers #447

merged 34 commits into from
Aug 21, 2023

Conversation

Edoardo-Pedicillo
Copy link
Contributor

@Edoardo-Pedicillo Edoardo-Pedicillo commented Jul 18, 2023

This PR ports the classifiers' module to the autocalibration framework.

Checklist:

  • Reviewers confirm new code works as expected.
  • Tests are passing.
  • Coverage does not decrease.
  • Documentation is updated.
  • Compatibility with Qibo modules (Please edit this section if the current pull request is not compatible with the following branches).
    • Qibo: master
    • Qibolab: main
    • Qibolab_platforms_qrc: main

@codecov
Copy link

codecov bot commented Jul 19, 2023

Codecov Report

Merging #447 (93f721c) into main (11e9198) will increase coverage by 0.11%.
Report is 2 commits behind head on main.
The diff coverage is 98.03%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #447      +/-   ##
==========================================
+ Coverage   95.83%   95.95%   +0.11%     
==========================================
  Files          55       61       +6     
  Lines        4106     4425     +319     
==========================================
+ Hits         3935     4246     +311     
- Misses        171      179       +8     
Flag Coverage Δ
unittests 95.95% <98.03%> (+0.11%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
src/qibocal/fitting/classifier/scikit_utils.py 83.33% <83.33%> (ø)
src/qibocal/fitting/classifier/qubit_fit.py 96.42% <96.42%> (ø)
src/qibocal/fitting/classifier/run.py 97.52% <97.52%> (ø)
src/qibocal/fitting/classifier/data.py 100.00% <100.00%> (ø)
src/qibocal/fitting/classifier/linear_svm.py 100.00% <100.00%> (ø)
src/qibocal/fitting/classifier/naive_bayes.py 100.00% <100.00%> (ø)
...bocal/protocols/characterization/classification.py 100.00% <100.00%> (ø)
...zation/readout_optimization/resonator_frequency.py 100.00% <100.00%> (ø)
src/qibocal/protocols/characterization/utils.py 95.04% <100.00%> (+0.21%) ⬆️

@Edoardo-Pedicillo Edoardo-Pedicillo marked this pull request as ready for review July 25, 2023 07:05
@rodolfocarobene
Copy link
Contributor

Thanks @Edoardo-Pedicillo, the code looks good to me (and finally it's fast!), I have just a couple of general comments:

  • we should add a detailed explanation of the routine in the documentation, with a list of the classifiers and a general explanation. For example, I did not really know which classifiers were available. Also, I have no idea what a ROC curve is or what AUC stands for... Maybe the names on the axis could help, but in any case I would also go for a documentation page
  • Maybe the plotting of the ROC curves and of the benchmarks should be plotted only if more than one classifiers is used
  • I got some warnings, I don't know if you can fix them / suppress them in some way. otherwise good anyway
The keyword argument 'parallel=True' was specified but no transformation for parallel execution was p
ossible.

To find out why, try turning on parallel diagnostics, see https://numba.readthedocs.io/en/stable/user
/parallel.html#diagnostics for help.

File "../qibocal/src/qibocal/protocols/characterization/utils.py", line 175:
@njit(["float64[:] (float64[:], float64[:])"], parallel=True, cache=True)
def cumulative(input_data, points):
^

  warnings.warn(errors.NumbaPerformanceWarning(msg,
/home/rodolfoc/venv/qibo/lib/python3.10/site-packages/skl2onnx/algebra/onnx_ops.py:159: UserWarning
: OpSchema.FormalParameter.typeStr is deprecated and will be removed in 1.16. Use OpSchema.FormalPa
rameter.type_str instead.
  tys = obj.typeStr or ''
/home/rodolfoc/venv/qibo/lib/python3.10/site-packages/skl2onnx/algebra/automation.py:154: UserWarni
ng: OpSchema.FormalParameter.isHomogeneous is deprecated and will be removed in 1.16. Use OpSchema.
FormalParameter.is_homogeneous instead.
  if getattr(obj, 'isHomogeneous', False):
/home/rodolfoc/venv/qibo/lib/python3.10/site-packages/jinja2/environment.py:485: UserWarning: OpSch
ema.FormalParameter.typeStr is deprecated and will be removed in 1.16. Use OpSchema.FormalParameter
.type_str instead.
  return getattr(obj, attribute)

Copy link
Contributor

@rodolfocarobene rodolfocarobene left a comment

Choose a reason for hiding this comment

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

There are some conflicts, but in general looks good to me

Copy link
Contributor

@andrea-pasquale andrea-pasquale left a comment

Choose a reason for hiding this comment

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

Thanks @Edoardo-Pedicillo!
Generally it looks good to me, I tested it with both RFSoC and qblox and it works.
A few minor comments.
Could you please also edit the example runcard by adding the new version of this protocol?

src/qibocal/protocols/characterization/classification.py Outdated Show resolved Hide resolved
src/qibocal/protocols/characterization/classification.py Outdated Show resolved Hide resolved
tests/runcards/protocols.yml Show resolved Hide resolved
src/qibocal/fitting/classifier/run.py Show resolved Hide resolved
src/qibocal/fitting/classifier/run.py Outdated Show resolved Hide resolved
Copy link
Contributor

@andrea-pasquale andrea-pasquale left a comment

Choose a reason for hiding this comment

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

Thanks for all the updates @Edoardo-Pedicillo.

I have added the show_contour parameter.

I think that maybe it would be better to show or not show directly the contour by toggling it on and off in the legend. By going to the previous I meant to show just the classification and the raw data without anything else (possibly by increasing the size of the plot).
The problem with having the show_contour parameter is that in order to show it or not show it you will need to rerun the experiment again.

I also have a small comment down below.

src/qibocal/protocols/characterization/classification.py Outdated Show resolved Hide resolved
Copy link
Contributor

@andrea-pasquale andrea-pasquale left a comment

Choose a reason for hiding this comment

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

@andrea-pasquale andrea-pasquale added this pull request to the merge queue Aug 21, 2023
Merged via the queue into main with commit 9874557 Aug 21, 2023
20 checks passed
@andrea-pasquale andrea-pasquale deleted the classifiers branch August 21, 2023 18:58
andrea-pasquale added a commit that referenced this pull request Aug 28, 2023
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.

3 participants