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

Add fidelities #906

Merged
merged 11 commits into from
Jun 10, 2024
Merged

Add fidelities #906

merged 11 commits into from
Jun 10, 2024

Conversation

Jacfomg
Copy link
Contributor

@Jacfomg Jacfomg commented May 21, 2024

Add fidelitiy attributes

Checklist:

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

src/qibolab/dummy/parameters.json Outdated Show resolved Hide resolved
src/qibolab/qubits.py Outdated Show resolved Hide resolved
src/qibolab/qubits.py Outdated Show resolved Hide resolved
src/qibolab/dummy/parameters.json Outdated Show resolved Hide resolved
@alecandido alecandido added this to the Qibolab 0.1.8 milestone May 22, 2024
Copy link

codecov bot commented May 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 66.72%. Comparing base (a964ab2) to head (397ce65).
Report is 16 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #906      +/-   ##
==========================================
+ Coverage   66.64%   66.72%   +0.07%     
==========================================
  Files          55       55              
  Lines        5954     5968      +14     
==========================================
+ Hits         3968     3982      +14     
  Misses       1986     1986              
Flag Coverage Δ
unittests 66.72% <100.00%> (+0.07%) ⬆️

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.

@Jacfomg Jacfomg mentioned this pull request May 22, 2024
8 tasks
src/qibolab/serialize.py Outdated Show resolved Hide resolved
src/qibolab/serialize.py Outdated Show resolved Hide resolved
src/qibolab/serialize.py Outdated Show resolved Hide resolved
Copy link
Member

@alecandido alecandido left a comment

Choose a reason for hiding this comment

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

I agree with @stavros11 comments. I'm adding a few style-related comments.

However, in general I'd try to have a fixed layout, and instead of making operations conditional on something (like the if pairs: branchings) I'd rather try to design the code to work smoothly in limit cases (i.e. if you get an empty dictionary, you just do nothing, and you shouldn't need any further conditional). Moreover, keeping a fixed layout makes it simpler to avoid branching.
However, in this case maintaining compatibility is likely more relevant, so please follow @stavros11 advice.

Comment on lines +23 to +24
"qubit1",
"qubit2",
Copy link
Member

Choose a reason for hiding this comment

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

Where are these names coming from?

Copy link
Member

Choose a reason for hiding this comment

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

qubit1: Qubit

since now EXCLUDE_FIELDS is also used in QubitPair
if fld.name not in EXCLUDED_FIELDS

Copy link
Member

Choose a reason for hiding this comment

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

I now wonder whether it would be a better option to have one each...

Btw, we don't even strictly need the EXCLUDED_FIELDS constant, but we could use field(..., metadata={"excluded": True}) or use Annotated[..., EXCLUDED] as type hint (defining an EXCLUDED constant), to keep it local to the ignored field.

Copy link
Member

@alecandido alecandido Jun 6, 2024

Choose a reason for hiding this comment

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

(a possible way of defining unique constants can be found in the standard library https://github.com/python/cpython/blob/b03d71d0d8415281e71ae4f1455222db4b4b66d2/Lib/dataclasses.py#L184-L186)

(at runtime, you could filter dataclass.fields() by using EXCLUDED not in field.type.__metadata__)

@@ -163,6 +172,11 @@ def dump_characterization(qubits: QubitMap, couplers: CouplerMap = None) -> dict
},
}

if pairs:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if pairs:
if pairs is not None:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pairs is an empty dictionary, if I make it a None when there is no qubit pairs it will start complaining in several places when it gets asked for values. So could I keep this to check for empty dictionaries ?

Copy link
Member

Choose a reason for hiding this comment

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

I guess he meant len(pairs) > 0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah sure, that I can do

src/qibolab/serialize.py Outdated Show resolved Hide resolved
src/qibolab/serialize.py Outdated Show resolved Hide resolved
@stavros11 stavros11 merged commit 417150b into main Jun 10, 2024
24 checks passed
@stavros11 stavros11 deleted the add_fidelities branch June 10, 2024 15:07
@Jacfomg
Copy link
Contributor Author

Jacfomg commented Jun 10, 2024

Thanks @stavros11

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.

4 participants