-
Notifications
You must be signed in to change notification settings - Fork 28
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
Rename top-level module to circuit_knitting
#244
Conversation
and also: - `entanglement_forging` -> `forging` - `circuit_cutting` -> `cutting`
Pull Request Test Coverage Report for Build 5183607457
💛 - Coveralls |
circuit_knitting
circuit_knitting
@@ -90,3 +39,35 @@ | |||
"PartitionedCuttingProblem", | |||
"CuttingExperimentResults", | |||
] | |||
|
|||
sys.modules["circuit_knitting_toolbox.circuit_cutting.qpd"] = qpd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain what is going on with setting these modules
? Not really familiar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It tells the module system that if somebody tries to import a module called circuit_knitting_toolbox.circuit_cutting.qpd
, then instead use this other thing that's already been loaded, which in this case is circuit_knitting.cutting.qpd
.
This is explained at bit at https://stackoverflow.com/a/72244240/1558890. (I tried the lazy method mentioned there too, but it had more issues than was worth.)
@@ -93,12 +93,18 @@ notebook-dependencies = [ | |||
"Documentation" = "https://qiskit-extensions.github.io/circuit-knitting-toolbox/" | |||
"Repository" = "https://github.com/Qiskit-Extensions/circuit-knitting-toolbox" | |||
|
|||
[tool.hatch.build.targets.wheel] | |||
only-include = [ | |||
"circuit_knitting", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So users will have access to two namespaces, and one of the namespaces will throw a bunch of warnings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really great, nice job. I had a couple questions out of interest but lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more thing, should we rename the docs directories to cutting
and forging
?
This was an intentional choice, but we could re-evaluate. Quoting from #221:
|
I agree with both. I don't think this is a source of confusion |
--- | ||
upgrade: | ||
- | | ||
The top-level name for imports has been renamed from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aren't deprecations normally replaced with a new target or a new way to migrate? How is this different? Isn't it a deprecation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would think the clean-up of the old package would be an upgrade, but I'm no authority on this :)
This performs the renames porposed in #221 and thus closes #221.
In order to demonstrate that the deprecated import locations work, I have not updated the imports in the tests or notebooks yet. I'll do that once CI passes.
I also made it so docs will no longer deploy for pushes to
main
, sincemain
will now be ahead of the current stable release.