-
Notifications
You must be signed in to change notification settings - Fork 21
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 tests to use qiskit_algorithms
#34
Conversation
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.
Overall lgtm, just one quick question about how we go about doing the skip.
tox.ini
Outdated
commands = | ||
stestr run {posargs} | ||
stestr run --exclude-regex 'test_ground_state_solver' {posargs} |
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.
Would it make more sense to have the test in question raise a skip exception or use the skip decorators https://docs.python.org/3/library/unittest.html#skipping-tests-and-expected-failures ?
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 would. Let me change that
This reverts commit dcb2a21.
@@ -27,6 +30,10 @@ | |||
class TestGroundStateSolvers(base.BaseTestCase): | |||
"""Test the use of the execute() method in qiskit-terra.""" | |||
|
|||
@unittest.skipIf( | |||
tuple(map(int, qiskit_nature.__version__.split("."))) < (0, 7, 0), |
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.
Qiskit nature doesn't do pre, post releases does it? This would error if the there version string were 0.8.0rc1 for example. Pep 440 documents valid version strings and it's not always just 3 period separate integers.
Actually now that I've thought about it a bit more, this also would break nature's ci if they're setting a dev version with the git sha1 from editable installs like we do in qiskit. As the dev versions have non int characters. For example on qiskit it's something like: 0.25.2.dev0+90a20e9
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.
Yes, I was cutting some corners assuming the 3 period separate integers. I have updated it to check the first 2 integers, which should always be there (this is also the way it was being done in qiskit_algorithms
now that I look at it). This is not accounting for the possibility of having epoch versions (as defined in PEP 440), but I don't think we would be using them in Nature so it felt like a bit of an overkill.
LGTM now, thanks for the update |
This PR is a pre-requirement for the removal of
qiskit.algorithms
fromqiskit
(Qiskit/qiskit#11086), but is currently blocked by the release of the latest version ofqiskit_nature
. As a temporary solution, this PR skips the nature test, which should be restored once the nature 0.7 release is out.