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

Rename top-level module to circuit_knitting #244

Merged
merged 4 commits into from
Jun 6, 2023
Merged

Conversation

garrison
Copy link
Member

@garrison garrison commented Jun 6, 2023

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, since main will now be ahead of the current stable release.

and also:
- `entanglement_forging` -> `forging`
- `circuit_cutting` -> `cutting`
@github-actions
Copy link

github-actions bot commented Jun 6, 2023

Pull Request Test Coverage Report for Build 5183607457

  • 16 of 16 (100.0%) changed or added relevant lines in 5 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 89.63%

Totals Coverage Status
Change from base Build 5183297038: 0.0%
Covered Lines: 2325
Relevant Lines: 2594

💛 - Coveralls

@garrison garrison marked this pull request as ready for review June 6, 2023 01:47
@garrison garrison changed the title Rename package to circuit_knitting Rename top-level module to circuit_knitting Jun 6, 2023
circuit_knitting/__init__.py Outdated Show resolved Hide resolved
@@ -90,3 +39,35 @@
"PartitionedCuttingProblem",
"CuttingExperimentResults",
]

sys.modules["circuit_knitting_toolbox.circuit_cutting.qpd"] = qpd
Copy link
Collaborator

@caleb-johnson caleb-johnson Jun 6, 2023

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

Copy link
Member Author

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",
Copy link
Collaborator

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

Exactly.

caleb-johnson
caleb-johnson previously approved these changes Jun 6, 2023
Copy link
Collaborator

@caleb-johnson caleb-johnson left a 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

Copy link
Collaborator

@caleb-johnson caleb-johnson left a 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 ?

@garrison
Copy link
Member Author

garrison commented Jun 6, 2023

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:

Let's keep the documentation directories the way they are rather than replace circuit_cutting and entanglement_forging with shorter names. This will

  • prevent links from unnecessarily breaking
  • provide more descriptive names to people navigating the documentation, e.g., the tutorials

@caleb-johnson
Copy link
Collaborator

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:

Let's keep the documentation directories the way they are rather than replace circuit_cutting and entanglement_forging with shorter names. This will

  • prevent links from unnecessarily breaking
  • provide more descriptive names to people navigating the documentation, e.g., the tutorials

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
Copy link
Collaborator

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?

Copy link
Collaborator

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 :)

@garrison garrison merged commit 4a0cd45 into main Jun 6, 2023
@garrison garrison deleted the rename-package branch June 6, 2023 17:19
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.

Shorten top-level module names
2 participants