"sampler" is currently a keyword argument for QNSPSA #218
Conversation
The example provided specifies the sampler option as if it would be positional, but it is actually a keyword argument. I don't understand the "*" or the possibility of the sampler to be None. But in order to make it work without removing those, the sampler hast to be especified as "sampler"=sampler.
"sampler" is currently a keyword argument
|
Hmm this could've been an unfortunate result of #17, which changed the signature from supporting either (1) a backend and an expectation (from the old opflow) or (2) a sampler, to only using the sampler. Since the sampler is now required it might be better to just make it a required argument, i.e. change the signature to def get_fidelity(ansatz: QuantumCircuit, sampler: BaseSampler) -> ... |
As suggested by @Cryoris, instead of the workaround on the example text, I updated the arguments of the get_fidelity method to include only the required elements and avoid confusion.
|
This change makes sense to me, this probably merits an |
|
@Cryoris As far as I can see, from quickly running mypy locally, is that See numpy/numpy#28076 where this aspect of 2.2 and failing for mypy is discussed, which I found once I had seen the above behavior. |
| - | | ||
| The `None` value for the `sampler` on the `QNSPSA.get_fidelity()` function | ||
| is no longer valid. A proper sampler must be provided. | ||
|
|
There was a problem hiding this comment.
My take would be all that is needed here would be an upgrade section - here are no deprecations (ie deprecated decorators added where function is kept for now and removed later - for instance with support of None which was simply removed at this time. There are no issues - the change fixes an issue but issues is for things we know fail in the release, ie current issues that were not addressed yet, nor are there any new major features added for a features sections. See the description of what goes in the sections here
https://docs.openstack.org/reno/latest/user/usage.html#editing-a-release-note
Now we could have a prelude but again it refers to deprecated - this text is combined with other release note preludes to make, in effect an overall intro for the release. I am not sure this is significant enough for that.
My take in upgrade would be to state that a sampler not must be provided as the default value of None has been removed. You can also note that the sampler has been made a positional argument so does not need to be passed using that as a keyword. FYI of course you can still do so if you wish, as in the following test case where calls get_fidelity() which
has not been changed and still works. I.e all the unit tests here in CI pass
Ran: 588 tests in 71.4273 sec.
- Passed: 574
- Skipped: 14
- Expected Fail: 0
- Unexpected Success: 0
- Failed: 0
CI overall is still failing overall though, as noted above, around mypy checking.
| The list of arguments for `QNSPSA.get_fidelity()` has been updated to | ||
| include only the `ansatz` and the `sampler`, and removing previous | ||
| wildcard option. From now on, the sampler option must be explicitly | ||
| especified, as a positional or keyword argument, and it cannot be `None`. |
There was a problem hiding this comment.
| especified, as a positional or keyword argument, and it cannot be `None`. | |
| specified, as a positional or keyword argument, and it cannot be `None`. |
Should fix the Checks spelling failure. In the other file the Checks are failing as the copyright year needs updating but it's too far away from the place that was changed to do a suggestion like I did above
Wrong Copyright Year:'qiskit_algorithms/optimizers/qnspsa.py': Current:'# (C) Copyright IBM 2021, 2024.' Correct:'# (C) Copyright IBM 2021, 2025.'
I had done another PR which fixed the other CI failures and having merged that and now updated this PR from main most is passing except the above 2 items. The CI failure against Qiskit Main is expected as there are a lot of changes needed to make it Qiskit 2.0 compatible, but those checks are not required to pass unlike the Checks which is required and is failing due to the above couple of items.
There was a problem hiding this comment.
As noted above, in the latter part of that comment, the copyright date of that changed file qnspsa.py needs updating otherwise, as you can see from the CI that just ran with the one spelling change applied, its still failing,
There was a problem hiding this comment.
Oh! Sorry about that...
…70947f3cd7.yaml Fixed typo Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>
Updated the Copyright year
Pull Request Test Coverage Report for Build 14126985232Details
💛 - Coveralls |
releasenotes/notes/sampler_as_positional_arg_for_qnspsa-2efca170947f3cd7.yaml
Outdated
Show resolved
Hide resolved
…70947f3cd7.yaml A more accurate rephrasing of the sentence, avoiding the misleading term "wildcard". Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>
Summary
The example provided for QNSPSA specifies the sampler option as if it would be positional, but it is actually a keyword argument.
I don't understand the "*" or the possibility of the sampler to be None. But in order to make it work without removing those, the sampler hast to be specified as "sampler"=sampler.
Details and comments