Skip to content

refactoring emulator results.py file - #1412

Merged
lballerio merged 18 commits into
mainfrom
mynewbranch
Apr 24, 2026
Merged

refactoring emulator results.py file#1412
lballerio merged 18 commits into
mainfrom
mynewbranch

Conversation

@lballerio

@lballerio lballerio commented Mar 26, 2026

Copy link
Copy Markdown
Contributor

In this small PR we simply anticipate the results.py modification that were first introduced in PR #1401.

Checklist:

  • Reviewers confirm new code works as expected.
  • Tests are passing.
  • Coverage does not decrease.
  • Documentation is updated.

@codecov

codecov Bot commented Mar 26, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.87234% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 37.54%. Comparing base (1df029e) to head (086b06d).
⚠️ Report is 19 commits behind head on main.

Files with missing lines Patch % Lines
src/qibolab/_core/instruments/emulator/emulator.py 90.90% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1412      +/-   ##
==========================================
+ Coverage   37.36%   37.54%   +0.18%     
==========================================
  Files         114      114              
  Lines        5736     5755      +19     
==========================================
+ Hits         2143     2161      +18     
- Misses       3593     3594       +1     
Flag Coverage Δ
unittests 37.54% <97.87%> (+0.18%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@lballerio
lballerio marked this pull request as ready for review March 26, 2026 13:56
@lballerio
lballerio requested review from a team March 26, 2026 13:56
@lballerio lballerio assigned alecandido and unassigned alecandido Mar 26, 2026
@lballerio
lballerio requested a review from alecandido March 26, 2026 14:06
Comment thread src/qibolab/_core/instruments/emulator/results.py Outdated
Comment thread src/qibolab/_core/instruments/emulator/results.py Outdated
Comment thread src/qibolab/_core/instruments/emulator/results.py
Comment thread tests/instruments/emulator/test_results.py
Comment thread src/qibolab/_core/instruments/emulator/emulator.py
Comment thread src/qibolab/_core/instruments/emulator/results.py Outdated
Comment thread src/qibolab/_core/instruments/emulator/emulator.py Outdated
Comment thread src/qibolab/_core/instruments/emulator/results.py Outdated
Comment thread src/qibolab/_core/instruments/emulator/results.py Outdated
Comment thread src/qibolab/_core/instruments/emulator/results.py
Comment thread src/qibolab/_core/instruments/emulator/emulator.py Outdated
Comment thread tests/instruments/emulator/platforms/fixed-frequency-qutrits/calibration.json Outdated
Comment thread src/qibolab/_core/instruments/emulator/results.py Outdated
Comment thread src/qibolab/_core/instruments/emulator/emulator.py Outdated
@lballerio
lballerio marked this pull request as draft March 26, 2026 15:52
@lballerio

Copy link
Copy Markdown
Contributor Author

Hi @alecandido, am I wrong or this np.stack call is superfluous?

np.stack(np.unravel_index(meas, hamiltonian.dims))[i]

and we can simply write:

np.unravel_index(meas, hamiltonian.dims)[i]

@alecandido

Copy link
Copy Markdown
Member

Yes, it looks irrelevant: np.unravel_index() is returning a tuple of arrays, and np.stack() concatenating them into a single one. If you were applying any other indexing operation, it would matter. But if you index over the most external one, the indexing of the array n + 1-dimensional array coincides with the indexing of the tuple of n-dimensional arrays

import numpy as np
a = tuple(np.arange(i*100, i*100 + 10) for i in range(10, 20))
np.stack(a)[3] == a [3]

(for higher dimensions, just .reshape() the result of np.arange())

@lballerio
lballerio marked this pull request as ready for review March 27, 2026 13:48
@lballerio
lballerio force-pushed the mynewbranch branch 2 times, most recently from dc7fd25 to da18ccf Compare March 27, 2026 14:04

@alecandido alecandido left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just reading it, it seems alright. If you also tested and are reasonably confident with the results, we could merge.

I only left few marginal comments.

Comment thread doc/source/main-documentation/emulator.rst Outdated
Comment on lines +255 to +256
if options.averaging_mode is AveragingMode.SINGLESHOT and options.nshots is None:
raise ValueError("nshots must be specified for SINGLESHOT mode")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Actually, since this only depends on the function inputs, better to place it as soon as possible in the function (instead of performing first some computation, to then fail anyhow).

Comment on lines +133 to +137
# Through the entire function state_probs has dimensions:
# (*S, M *H_dim), where
# *S is the number of iteration for each sweep in the experiment
# M is the number of measurements applied in the pulse sequence
# *H_dim is the complete system dimension

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

As suggested before, move this explanation in the module-level docstring. And then just comment locally using the defined symbols

@lballerio
lballerio requested a review from alecandido April 14, 2026 07:58
@lballerio
lballerio force-pushed the mynewbranch branch 2 times, most recently from 62f6081 to 7473480 Compare April 23, 2026 09:16
@lballerio
lballerio added this pull request to the merge queue Apr 24, 2026
Merged via the queue into main with commit 51e1790 Apr 24, 2026
66 of 87 checks passed
@lballerio
lballerio deleted the mynewbranch branch April 24, 2026 06:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants