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

Setting qutip-qip as an optional plugin for qutip-5 [unitaryhack] #1920

Merged
merged 7 commits into from Jun 8, 2022

Conversation

HGSilveri
Copy link
Contributor

@HGSilveri HGSilveri commented Jun 6, 2022

Description

  • Allow imports of qutip.qip seamlessly as imports of qutip_qip
  • Test the import behaviour when qutip_qip is installed and when not.

Related issues or PRs
Incorporates the ideas in the discussion of #1500.
Fixes #1893.

Todos and questions

  • Have qutip-qip be an optional dependency (will be left for a future PR)
  • Add installation of qutip-qip to the CI workflow

I have a draft of these changes locally, but they fail due to a dependency conflict: qutip_qip currently requires qutip<5 and >=4.6 while these changes are being implemented in v5.0.0.dev, ie >5. This will cause the unit test where qutip_qip is installed to be skipped. Any ideas on how to get around this?

Edit: Uses a stub for the qutip_qip package in the units tests instead, which removes the need of having qutip-qip installed.

Changelog

Allow imports of qutip.qip seamlessly as imports of qutip_qip.
Have qutip-qip be an optional dependency.

@BoxiLi
Copy link
Member

BoxiLi commented Jun 6, 2022

Ah yes, that constraint was there to prevent accidentally using qutip-qip with qutip@dev.major because the support was only added a few weeks ago. Only the master branch of qutip-qip support this. So this basically prevents installing the qutip@dev.major and the released qutip-qip at the same time. One has to install qutip-qip@master to test them.

This also means that adding qutip-qip as an optional package right now does not make much sense. Probably we can drop this.

To test it on GitHub action we need to install qutip-qip@master for now and test the importation. @hodgestar Is that ok? Otherwise, I can try to get out a new release of qutip-qip in a day or so. Just to update the change log and click the button.

@HGSilveri
Copy link
Contributor Author

This also means that adding qutip-qip as an optional package right now does not make much sense. Probably we can drop this.

To test it on GitHub action we need to install qutip-qip@master for now and test the importation. @hodgestar Is that ok? Otherwise, I can try to get out a new release of qutip-qip in a day or so. Just to update the change log and click the button.

Completely up to you, of course! If you choose to drop the optional dependency for now, I can also just put those changes on another PR to be merged when the time is right.
I'll stand by for your decision regarding the CI tests.

@coveralls
Copy link

coveralls commented Jun 6, 2022

Coverage Status

Coverage increased (+0.2%) to 65.507% when pulling 2049dfd on HGSilveri:qip-module into 9bb7c7a on qutip:dev.major.

Copy link
Contributor

@hodgestar hodgestar left a comment

Choose a reason for hiding this comment

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

I made some suggestions to make it possible to always run the tests.

qutip/qip.py Outdated Show resolved Hide resolved
qutip/qip.py Outdated Show resolved Hide resolved
qutip/tests/test_qip.py Outdated Show resolved Hide resolved
qutip/tests/test_qip.py Outdated Show resolved Hide resolved
qutip/tests/test_qip.py Outdated Show resolved Hide resolved
@hodgestar
Copy link
Contributor

Regarding the suggestions in the TODO list:

Have qutip-qip be an optional dependency

I'm wondering about this. Having pip install qutip[qip] is nice, but what should happen with pip install qutip[full]? Should that also install qutip_qip? If not should we then leave qutip_qip out of full? And should we have qutip[full-family]?

Add installation of qutip-qip to the CI workflow

This we should not do -- we don't want anything in core qutip to accidentally require qutip_qip (that was sort of the point of the family packages).

@BoxiLi
Copy link
Member

BoxiLi commented Jun 6, 2022

I'm wondering about this. Having pip install qutip[qip] is nice, but what should happen with pip install qutip[full]? Should that also install qutip_qip? If not should we then leave qutip_qip out of full? And should we have qutip[full-family]?

I understand [full] as "installing everything that might be needed so that I can run everything without any ImportError". So this means that it includes [qip]. However, I do get your concern that this will probably make the option [full] larger and larger in the future.

Nonetheless, since there is not yet a released qutip-qip that supports qutip-5. Let's leave it to a different PR.

@HGSilveri
Copy link
Contributor Author

This we should not do -- we don't want anything in core qutip to accidentally require qutip_qip (that was sort of the point of the family packages).

Sure, no need for that anymore, I'll edit the description.

@hodgestar
Copy link
Contributor

I understand [full] as "installing everything that might be needed so that I can run everything without any ImportError". So this means that it includes [qip]. However, I do get your concern that this will probably make the option [full] larger and larger in the future.

Perhaps installing qutip_qip as part of full is a good first step. It means that QuTiP 5 will just "work" as it did before and we can always remove it from full in QuTiP 5.1 or later.

@HGSilveri HGSilveri requested a review from hodgestar June 7, 2022 08:58
Copy link
Contributor

@hodgestar hodgestar left a comment

Choose a reason for hiding this comment

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

I had a few more ideas about how to test this nicely.

qutip/qip.py Outdated Show resolved Hide resolved
qutip/tests/test_qip.py Show resolved Hide resolved
qutip/tests/test_qip.py Outdated Show resolved Hide resolved
Copy link
Contributor

@hodgestar hodgestar left a comment

Choose a reason for hiding this comment

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

Looks good!

@hodgestar
Copy link
Contributor

@BoxiLi I'd like you to approve to since this is a very important link to qutip_qip.

Once we have Boxi's approval and the tests pass, I think we can consider this done.

Copy link
Member

@BoxiLi BoxiLi left a comment

Choose a reason for hiding this comment

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

Looks great! Also tested it locally with/without an installed qutip-qip.

@hodgestar hodgestar merged commit 781d58c into qutip:dev.major Jun 8, 2022
@hodgestar hodgestar added the unitaryhack-accepted This bounty for this UnitaryHack issue should be paid. label Jun 8, 2022
@HGSilveri HGSilveri deleted the qip-module branch June 9, 2022 07:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
unitaryhack-accepted This bounty for this UnitaryHack issue should be paid.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants