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

[Refactoring]: split devices.noise_utils into qis.noise_utils and devices.noise_utils #6453

Merged
merged 12 commits into from
Feb 9, 2024

Conversation

NoureldinYosri
Copy link
Collaborator

Some of the functions in devices.noise_utils do conversion between different errors. these functions are not specific to devices but fall more appropriately under qis.

This change is because these functions will be used in cirq.experiments


I added the BREAKING CHANGE label because if it will break users of these functions who will have to import the functions from qis rather than devices.

@CirqBot CirqBot added the size: L 250< lines changed <1000 label Feb 8, 2024
@NoureldinYosri NoureldinYosri changed the title [Refactoring]: split devices.noise_utils into qis.noise_utils and devices.noise_utils [Refactoring]: split devices.noise_utils into qis.noise_utils and devices.noise_utils Feb 8, 2024
@NoureldinYosri NoureldinYosri added the BREAKING CHANGE For pull requests that are important to mention in release notes. label Feb 8, 2024
@NoureldinYosri NoureldinYosri marked this pull request as ready for review February 8, 2024 22:15
@NoureldinYosri NoureldinYosri requested review from vtomole, cduck and a team as code owners February 8, 2024 22:15
Copy link
Collaborator

@eliottrosenberg eliottrosenberg left a comment

Choose a reason for hiding this comment

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

LGTM with nits

@@ -0,0 +1,123 @@
# Copyright 2021 The Cirq Developers
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we update the year?

@@ -0,0 +1,97 @@
# Copyright 2021 The Cirq Developers
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same comment

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't know. I'm not creating new content. it's the same file. @pavoljuhas what do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think either way is fine in this case, feel free to leave it as is.
For new module with new code we should use current year.

Copy link
Collaborator

@pavoljuhas pavoljuhas left a comment

Choose a reason for hiding this comment

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

I added the BREAKING CHANGE label because if it will break users of these functions who will have to import the functions from qis rather than devices.

Can you add deprecated proxies to the moved functions at their original modules?

The @deprecated decorator at cirq._compat should help with that.

@NoureldinYosri
Copy link
Collaborator Author

@pavoljuhas ptal

@NoureldinYosri NoureldinYosri enabled auto-merge (squash) February 9, 2024 00:48
@pavoljuhas pavoljuhas disabled auto-merge February 9, 2024 01:01
Copy link
Collaborator

@pavoljuhas pavoljuhas left a comment

Choose a reason for hiding this comment

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

LGTM with updated import name suggestion

@NoureldinYosri NoureldinYosri enabled auto-merge (squash) February 9, 2024 01:08
@NoureldinYosri NoureldinYosri merged commit f1aacd7 into main Feb 9, 2024
35 checks passed
harry-phasecraft pushed a commit to PhaseCraft/Cirq that referenced this pull request Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BREAKING CHANGE For pull requests that are important to mention in release notes. size: L 250< lines changed <1000
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants