-
Notifications
You must be signed in to change notification settings - Fork 639
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
Added about() functionality for qutip_qip #1870
base: master
Are you sure you want to change the base?
Conversation
Thanks @claretgrace0801 for the PR. This is nice indeed that qutip_qip only needs to call As it is now, however, every new qutip family package needs to modify this to add their own info. I'm wondering if we can modify the code so that this only needs to be done once in It could be implemented e.g.
Also, it would be helpful to add a test in the |
I've made the changes. Now other libraries won't have to change anything in |
I just realize that test in qutip-qip won't work because this has to be merged and released first. The code looks fine to me. Will test it locally later. @hodgestar Just to get a second opinion, what do you think about this? As it could also be used by further family packages like control. |
@BoxiLi I'm -1 on adding such complexity to I would suggest a simpler solution, which is to have |
I see your point that we should keep those debugging tools as simple as possible. Instead of reading the version in Something like def about(family_pkgs=[]):
...
for family_pkg in family_pkgs:
package_name, package_version = family_pkg._about_family_pkg()
print(....) In this way we don't need to pass the string name of the package but just pass the package itself. If anything went wrong, it is just in the implementation of |
At the expense of some complication, we could add entry_points to the family packages and then |
Pseudocode for entrypoint suggestion: entrypoints = importlib.metadata.entrypoints(group="qutip.about")
for ep in entrypoints:
about_func = ep.load()
try:
title, lines = about_func()
except Exception as exc:
title, lines = ep.name, [str(exc)]
print(title)
print("-" * title)
print()
for line in lines:
print(line)
print() |
Hi @claretgrace0801, could you have a look at this entrypoint approach and try to implement it? Something similar to the example that @hodgestar provided above. The advantage of this There is also some discussion of entrypoint in #1500, but there we were talking about registering the whole qutip_qip package, here only the |
Sounds good. I'll try to implement the entrypoint approach, like what @hodgestar suggested. |
Hi @claretgrace0801, Any updates on this? |
Sorry for the delay. I'll make a commit in a few days. |
Hey @BoxiLi, please have a look at this. I've also committed to qutip/qutip-qip#138. |
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.
@claretgrace0801 Thanks!
Looks good to me. A few minor suggestions.
Could you also add a test for this? You can follow the method used #1920 which mocks a family package.
@@ -61,6 +63,19 @@ def about(): | |||
qutip_install_path = os.path.dirname(inspect.getsourcefile(qutip)) | |||
print("Installation path: %s" % qutip_install_path) | |||
|
|||
# family packages | |||
entrypoints = pkg_resources.iter_entry_points('qutip.about') |
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 is better to use importlib rather than pkg_resources
as stated in their documentation. The latter might be deprecated in the future.
print() | ||
print(title) | ||
for line in lines: | ||
print(line) |
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.
print(line) | |
print(line) | |
print() |
Add an empty line incase there are other family packages
Description
For solving issue qutip/qutip-qip#51, I have added an
about
functionality forqutip_qip
. Will also make the necessary PR in the qutip-qip repository for completing this issue.Related issues or PRs
fixes qutip/qutip-qip#51
Changelog
Added a parameter to
qutip
's about function which defaults to "qutip".qutip
's about can be called fromqutip_qip
with the argument of "qutip_qip" which will display information aboutqutip_qip
.Displaying
qutip_qip
's version and installation path.