This repository has been archived by the owner on Jun 12, 2023. It is now read-only.
Fix issues with CVXPY default solvers #422
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
chriseclectic
added
Changelog: Bugfix
mtreinish
reviewed
Jun 16, 2020
mtreinish
approved these changes
Jun 17, 2020
chriseclectic
added a commit
to chriseclectic/qiskit-ignis
that referenced
this pull request
Jun 17, 2020
- Fixes deprecated multiplication operators - Adds a check to set default SDP solver based on installed solvers in cvxpy, preferring 'CVXOP' if installed, then 'SCS'. - Changes auto method of tomography fitter to only use 'cvx' when a default solver is sound. When checking if 'SCS' is installed, this runs a short optimization problem to see if SCS is built with BLAS support, since it cannot solve problems bigger than 2x2 matrices without it. * Change tomography solver to check for installed CVX solvers * Fix cvxpy deprecation warnings * Fix check for SDP solver * Add cvxopt solver to test envs * Fix typo * and another * Install cvxopt from conda-forge
mtreinish
added a commit
to mtreinish/qiskit-ignis
that referenced
this pull request
Jun 19, 2020
In qiskit-community#422 the azure pipelines jobs were updated to simplify the dependency installation to no longer require build scs against openblas. However there was a bug in that which caused the jobs to not really run, but still return success. This commit corrects the oversight so we're still running CI for windows and actually testing it.
Merged
mtreinish
added a commit
to mtreinish/qiskit-ignis
that referenced
this pull request
Jun 19, 2020
This commit fixes an issue in the process tomography module. When cvxpy was not installed the module would fail to import. That's because the selection logic was reworked in qiskit-community#422 but never updated in process tomography to reflect that change. Since no CI job runs without cvxpy installed we never caught this edge case. This commit fixes the underlying issue to rework the process tomography fitter selection logic to mirror the changes to state tomography in qiskit-community#422 and then also try and add ci coverage it removes cvxpy from the docs tox job. With warnings set to fatal this should ensure we are always able to import everything and build the docs if cvxpy is not installed. Fixes qiskit-community#429
chriseclectic
pushed a commit
that referenced
this pull request
Jun 19, 2020
In #422 the azure pipelines jobs were updated to simplify the dependency installation to no longer require build scs against openblas. However there was a bug in that which caused the jobs to not really run, but still return success. This commit corrects the oversight so we're still running CI for windows and actually testing it.
mtreinish
added a commit
that referenced
this pull request
Jun 22, 2020
This commit fixes an issue in the process tomography module. When cvxpy was not installed the module would fail to import. That's because the selection logic was reworked in #422 but never updated in process tomography to reflect that change. Since no CI job runs without cvxpy installed we never caught this edge case. This commit fixes the underlying issue to rework the process tomography fitter selection logic to mirror the changes to state tomography in #422 and then also try and add ci coverage it removes cvxpy from the docs tox job. With warnings set to fatal this should ensure we are always able to import everything and build the docs if cvxpy is not installed. Fixes #429
mtreinish
added a commit
to mtreinish/qiskit-ignis
that referenced
this pull request
Jun 22, 2020
This commit fixes an issue in the process tomography module. When cvxpy was not installed the module would fail to import. That's because the selection logic was reworked in qiskit-community#422 but never updated in process tomography to reflect that change. Since no CI job runs without cvxpy installed we never caught this edge case. This commit fixes the underlying issue to rework the process tomography fitter selection logic to mirror the changes to state tomography in qiskit-community#422 and then also try and add ci coverage it removes cvxpy from the docs tox job. With warnings set to fatal this should ensure we are always able to import everything and build the docs if cvxpy is not installed. Fixes qiskit-community#429 (cherry picked from commit 01e9070)
mtreinish
added a commit
to mtreinish/qiskit-ignis
that referenced
this pull request
Jun 22, 2020
This commit adds a new ci job for running ignis tests without any optional dependencies. There are several optional dependencies which do not not always get installed with ignis. They are needed to enable optional features but shouldn't be required, we've had a slew of recent bugs around accidentally requiring these optional dependencies (see issues qiskit-community#429, qiskit-community#422, and qiskit-community#312). None of these were caught in CI because we always install all optional dependencies in CI test jobs. By adding a new job which explicitly installs the bare minimum we're emulating what a user does when they install just ignis. As part of this a missing dependency was added to the requirements list. Ignis has a hard dependency on scikit learn for the measurement discriminators, but this was never explicitly listed. This was never caught because the test jobs were always installing it.
mtreinish
added a commit
to mtreinish/qiskit-ignis
that referenced
this pull request
Jun 22, 2020
This commit adds a new ci job for running ignis tests without any optional dependencies. There are several optional dependencies which do not not always get installed with ignis. They are needed to enable optional features but shouldn't be required, we've had a slew of recent bugs around accidentally requiring these optional dependencies (see issues qiskit-community#429, qiskit-community#422, and qiskit-community#312). None of these were caught in CI because we always install all optional dependencies in CI test jobs. By adding a new job which explicitly installs the bare minimum we're emulating what a user does when they install just ignis. As part of this a missing dependency was added to the requirements list. Ignis has a hard dependency on scikit learn for the measurement discriminators, but this was never explicitly listed. This was never caught because the test jobs were always installing it.
chriseclectic
pushed a commit
that referenced
this pull request
Jun 22, 2020
This commit fixes an issue in the process tomography module. When cvxpy was not installed the module would fail to import. That's because the selection logic was reworked in #422 but never updated in process tomography to reflect that change. Since no CI job runs without cvxpy installed we never caught this edge case. This commit fixes the underlying issue to rework the process tomography fitter selection logic to mirror the changes to state tomography in #422 and then also try and add ci coverage it removes cvxpy from the docs tox job. With warnings set to fatal this should ensure we are always able to import everything and build the docs if cvxpy is not installed. Fixes #429 (cherry picked from commit 01e9070)
chriseclectic
pushed a commit
that referenced
this pull request
Jun 22, 2020
…ies (#436) * Add scikit-learn dependency and add CI job without optional deps This commit adds a new ci job for running ignis tests without any optional dependencies. There are several optional dependencies which do not not always get installed with ignis. They are needed to enable optional features but shouldn't be required, we've had a slew of recent bugs around accidentally requiring these optional dependencies (see issues #429, #422, and #312). None of these were caught in CI because we always install all optional dependencies in CI test jobs. By adding a new job which explicitly installs the bare minimum we're emulating what a user does when they install just ignis. As part of this a missing dependency was added to the requirements list. Ignis has a hard dependency on scikit learn for the measurement discriminators, but this was never explicitly listed. This was never caught because the test jobs were always installing it. * Don't install cvxopt in no-opt job * Add job name
mtreinish
added a commit
to mtreinish/qiskit-ignis
that referenced
this pull request
Jun 22, 2020
…ies (qiskit-community#436) * Add scikit-learn dependency and add CI job without optional deps This commit adds a new ci job for running ignis tests without any optional dependencies. There are several optional dependencies which do not not always get installed with ignis. They are needed to enable optional features but shouldn't be required, we've had a slew of recent bugs around accidentally requiring these optional dependencies (see issues qiskit-community#429, qiskit-community#422, and qiskit-community#312). None of these were caught in CI because we always install all optional dependencies in CI test jobs. By adding a new job which explicitly installs the bare minimum we're emulating what a user does when they install just ignis. As part of this a missing dependency was added to the requirements list. Ignis has a hard dependency on scikit learn for the measurement discriminators, but this was never explicitly listed. This was never caught because the test jobs were always installing it. * Don't install cvxopt in no-opt job * Add job name * Update tox.ini to point to remove master terra for stable branch (cherry picked from commit f0b68e4)
chriseclectic
pushed a commit
that referenced
this pull request
Jun 22, 2020
…ies (#436) (#439) * Add scikit-learn dependency and add CI job without optional deps This commit adds a new ci job for running ignis tests without any optional dependencies. There are several optional dependencies which do not not always get installed with ignis. They are needed to enable optional features but shouldn't be required, we've had a slew of recent bugs around accidentally requiring these optional dependencies (see issues #429, #422, and #312). None of these were caught in CI because we always install all optional dependencies in CI test jobs. By adding a new job which explicitly installs the bare minimum we're emulating what a user does when they install just ignis. As part of this a missing dependency was added to the requirements list. Ignis has a hard dependency on scikit learn for the measurement discriminators, but this was never explicitly listed. This was never caught because the test jobs were always installing it. * Don't install cvxopt in no-opt job * Add job name * Update tox.ini to point to remove master terra for stable branch (cherry picked from commit f0b68e4)
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Details and comments
When checking if 'SCS' is installed, this runs a short optimization problem to see if SCS is built with BLAS support, since it cannot solve problems bigger than 2x2 matrices without it.