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

Update about() and document latest package support #1220

Closed
wants to merge 14 commits into from

Conversation

crazy4pi314
Copy link
Contributor

@crazy4pi314 crazy4pi314 commented Apr 12, 2022

Fixes #1201

Description

Please explain the changes you made here.

Checklist

Check off the following once complete (or if not applicable) after opening the PR. The PR will be reviewed once this checklist is complete and all tests are passing.

If some items remain, you can mark this a draft pull request.

License

  • I license this contribution under the terms of the GNU GPL, version 3 and grant Unitary Fund the right to provide additional permissions as described in section 7 of the GNU GPL, version 3.

Tips

  • If the validation check fails:

    1. Run make check-types (from the root directory of the repository) and fix any mypy errors.

    2. Run make check-style and fix any flake8 errors.

    3. Run make format to format your code with the black autoformatter.

    For more information, check the Mitiq style guidelines.

  • Write "Fixes #XYZ" in the description if this PR fixes Issue #XYZ.

@github-actions
Copy link

Binder 👈 Launch a binder notebook on branch unitaryfund/mitiq/crazy4pi314/issue1201

@codecov
Copy link

codecov bot commented Apr 12, 2022

Codecov Report

Merging #1220 (b685e90) into master (f0303d6) will decrease coverage by 0.42%.
The diff coverage is 65.67%.

@@            Coverage Diff             @@
##           master    #1220      +/-   ##
==========================================
- Coverage   98.13%   97.71%   -0.43%     
==========================================
  Files          60       60              
  Lines        2737     2796      +59     
==========================================
+ Hits         2686     2732      +46     
- Misses         51       64      +13     
Impacted Files Coverage Δ
mitiq/_about.py 66.66% <65.15%> (+30.30%) ⬆️
mitiq/__init__.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f0303d6...b685e90. Read the comment docs.

@crazy4pi314 crazy4pi314 marked this pull request as ready for review April 18, 2022 18:23
@crazy4pi314
Copy link
Contributor Author

From #1201 there is still a few things left that this does not do, and I am a bit stuck on.

Add warning to QPROGRAM type builder
I am not sure the user experience we are looking for on this/where would be the right place to handle. It seems like it would be too annoying to put anytime a conversion is called, and it would already alert the user at the time of mitiq import. Maybe that is sufficient?

Add latest_supported() with output to docs under Supported Frontends
I have exported a function for user use called latest_supported_packages, but code cannot be executed in the readme. If we want that, we could have a script that greps the markdown file for versions and replaces them, but that seems tedious. Anyone have other options/suggestions? I just hardcoded them for now...

Copy link
Member

@andreamari andreamari left a comment

Choose a reason for hiding this comment

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

Thanks @crazy4pi314, the about() output looks is very good and clear!
The main problem is that we should access the requirements.txt and dev_requirements.txt files in a path independent way otherwise we get errors when using Mitiq from arbitrary folders.

mitiq/_about.py Outdated
# Logging versions of Mitiq's core dependencies (must be installed)
LATEST_SUPPORTED_PKGS = {
req.project_name: req.specs[0][1]
for req in parse_requirements(open("requirements.txt"))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
for req in parse_requirements(open("requirements.txt"))
for req in parse_requirements(open("requirements.txt"))

This works only if you are in the mitiq root folder. Otherwise the file is not found and one gets an error.

mitiq/_about.py Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@andreamari
Copy link
Member

From #1201 there is still a few things left that this does not do, and I am a bit stuck on.

Add warning to QPROGRAM type builder
I am not sure the user experience we are looking for on this/where would be the right place to handle. It seems like it would be too annoying to put anytime a conversion is called, and it would already alert the user at the time of mitiq import. Maybe that is sufficient?

I agree the current approach is good and is sufficient.

Add latest_supported() with output to docs under Supported Frontends
I have exported a function for user use called latest_supported_packages, but code cannot be executed in the readme. If we want that, we could have a script that greps the markdown file for versions and replaces them, but that seems tedious. Anyone have other options/suggestions? I just hardcoded them for now...

Fine to hardcode versions or to just remove them.
Another option is to call latest_supported() in a myst file of the docs that is different from the readme. But I am not sure if there is a good place for it in the current structure. Maybe somewhere in this page?

@crazy4pi314
Copy link
Contributor Author

The mypy issues are stumping me, I think I have created a race condition with the package loading process... :(

@andreamari
Copy link
Member

This PR got more involved than expected. I also tried to fix some aspects at some point but the situation is complicated since dev requirements are not easily accessible from pip versions of Mitiq (from PyPy).

Maybe we could close it in favor of a new PR which is focused on the simplified issue #1478.

@andreamari
Copy link
Member

Based on the discussion in #1478, we can close this PR. See #1478 (comment) therein for a good summary.

@andreamari andreamari closed this Oct 3, 2022
@natestemen natestemen deleted the crazy4pi314/issue1201 branch October 23, 2022 06:13
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.

Update about() and document latest package support
2 participants