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

qq compare command to compare two qibocal reports #714

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

GabrielePalazzo
Copy link
Contributor

@GabrielePalazzo GabrielePalazzo commented Feb 15, 2024

Example of a combined report containing a resonator spectroscopy and a resonator flux dependence:
http://login.qrccluster.com:9000/T5rbRzsmTRmKLtOSaa03DQ==

Only experiments with the same id are compared. All other experiments are discarded.
Tables contain results from both experiments side-by-side.
Experiments with scatterplots (e.g.: resonator spectroscopy) are shown in the same figure (with overlapping points and lines).
Plots of experiments with heatmaps (e.g.: resonator punchout) are vertically stacked.

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

Copy link

codecov bot commented Feb 15, 2024

Codecov Report

Attention: Patch coverage is 98.30508% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 97.21%. Comparing base (ebd19c8) to head (cb2fd0c).
Report is 272 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #714      +/-   ##
==========================================
+ Coverage   97.15%   97.21%   +0.05%     
==========================================
  Files         100      101       +1     
  Lines        7415     7533     +118     
==========================================
+ Hits         7204     7323     +119     
+ Misses        211      210       -1     
Flag Coverage Δ
unittests 97.21% <98.30%> (+0.05%) ⬆️

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

Files Coverage Δ
src/qibocal/cli/_base.py 92.30% <100.00%> (+1.07%) ⬆️
src/qibocal/cli/compare.py 98.18% <98.18%> (ø)

... and 1 file with indirect coverage changes

@andrea-pasquale
Copy link
Contributor

Missing lxml dependency.

@Edoardo-Pedicillo
Copy link
Contributor

Hi @GabrielePalazzo, what is the status of this PR ?

@GabrielePalazzo
Copy link
Contributor Author

Hi @GabrielePalazzo, what is the status of this PR ?

Comparison of single sweeper experiments (like resonator spectroscopy) works.
Flux experiments or readout characterization are not working (we should probably not compare heatmap plots). If I have some spare time later this week I’ll try to make it ready for review.

@Edoardo-Pedicillo
Copy link
Contributor

Edoardo-Pedicillo commented Apr 17, 2024

Flux experiments or readout characterization are not working (we should probably not compare heatmap plots)

I agree. If you want, you can just put the plots of the two reports near each other.

@andrea-pasquale
Copy link
Contributor

Flux experiments or readout characterization are not working (we should probably not compare heatmap plots)

I agree. If you want, you can just put the plots of the two reports near each other.

I was going to suggest the same. Another possible idea would be to come up with some sort of ratio plot to see the differences between the two outcomes. I don't know how feasible it is.

@GabrielePalazzo GabrielePalazzo marked this pull request as ready for review April 29, 2024 10:16
@GabrielePalazzo
Copy link
Contributor Author

The new code is not covered by tests. I'll try to write some.

@Edoardo-Pedicillo
Copy link
Contributor

@GabrielePalazzo, could you resolve the conflicts? Thanks

@andrea-pasquale
Copy link
Contributor

@GabrielePalazzo, could you resolve the conflicts? Thanks

For conflicts let me know if you need help. I recently changed some stuff related to the report generation.

@@ -36,6 +36,7 @@ seaborn = { version = "^0.12.2", optional = true }
pydot = { version = "^1.4.2", optional = true }
skl2onnx = { version = "^1.14.0", optional = true }
pyyaml = "^6.0"
lxml = "^5.1.0"
Copy link
Member

Choose a reason for hiding this comment

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

Why not with standard library? (maybe I just forgot...)

It seems reliable, and it's only for reporting, so it should be alright (at some point, we could decouple the UI from the library - CLI included - and this dependency would just be in the front-end), but one (compiled) dependency less is always better :P

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, for some reason I found it easier to use lxml for manipulating the html code, but I can try to switch to a standard package.

Comment on lines +136 to +141
html_block = lxml.html.fromstring(plots[0][0].to_html())
html_block[1].remove(html_block[1][0])
for i, plot in enumerate(plots):
single_report_html_block = lxml.html.fromstring(plot[0].to_html())
html_block[1].insert(i, single_report_html_block[1][0])
fig = lxml.etree.tostring(html_block).decode("ascii")
Copy link
Member

Choose a reason for hiding this comment

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

In addition to my previous comment.

The explicit HTML manipulation seems to be localized here.
Isn't it possible to avoid the HTML manipulation at all?

Moreover, it seems that the function is:

  • extracting the original HTML from the first plot, then into an object
  • removing the plot itself (I guess)
  • reinserting all the plots in the HTML as consecutive nodes
  • converting back to an HTML string

Can't you just manipulate the Plotly objects, and stack them vertically?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From what I remember, vertically stacking two plots from HTML reports was giving weird layouts (e.g.: plots overlapping one on top of the other).

Copy link
Member

Choose a reason for hiding this comment

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

I mean, doing something like this
https://plotly.com/python/subplots/#stacked-subplots

Since, initially, you should have the Plotly objects (those on which you're calling the .to_html() method)

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.

None yet

4 participants