Skip to content

Conversation

jakelishman
Copy link
Member

Summary

This new pass added in gh-10634 uses some deprecated Numpy properties and has some slightly fragile exception-based logic when the required properties can be directly tested. This code issues warnings with Numpy 1.25+, which is currently not used by CI due to gh-10305.

Details and comments

This should fix the common errors in #11020. There's still a Linux / Numpy 1.26.1 failure in unitary synthesis that's new, and it's not immediately clear to me if that's related or not.

I submitted this as a separate PR to #11020 because I'd like the history / CI to record that we're not sacrificing compatibility with Numpy 1.24 to add compatibility with 1.26.

This new pass added in Qiskitgh-10634 uses some deprecated Numpy properties
and has some slightly fragile exception-based logic when the required
properties can be directly tested.  This code issues warnings with Numpy
1.25+, which is currently not used by CI due to Qiskitgh-10305.
@jakelishman jakelishman added the Changelog: None Do not include in changelog label Oct 16, 2023
@jakelishman jakelishman requested a review from a team as a code owner October 16, 2023 12:51
@qiskit-bot
Copy link
Collaborator

One or more of the the following people are requested to review this:

  • @Qiskit/terra-core

@coveralls
Copy link

Pull Request Test Coverage Report for Build 6533835675

  • 8 of 8 (100.0%) changed or added relevant lines in 1 file are covered.
  • 13 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.01%) to 87.035%

Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/parse.rs 6 96.67%
crates/qasm2/src/lex.rs 7 90.4%
Totals Coverage Status
Change from base Build 6529876136: -0.01%
Covered Lines: 74417
Relevant Lines: 85502

💛 - Coveralls

mtreinish
mtreinish previously approved these changes Oct 16, 2023
Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

LGTM, I think this makes sense to me. TBH, I'm getting a bit lost in the logic on the old side but since it's passing tests I think we are fine.

@mtreinish mtreinish added this pull request to the merge queue Oct 16, 2023
@jakelishman jakelishman removed this pull request from the merge queue due to a manual request Oct 16, 2023
Copy link
Contributor

@ElePT ElePT left a comment

Choose a reason for hiding this comment

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

Approving to get it back to the merge queue.

@ElePT ElePT added this pull request to the merge queue Oct 23, 2023
Merged via the queue into Qiskit:main with commit 6bf90fa Oct 23, 2023
@jakelishman jakelishman deleted the fix-normalizerxangles branch October 23, 2023 11:24
@mtreinish mtreinish added the stable backport potential The bug might be minimal and/or import enough to be port to stable label Oct 23, 2023
mergify bot pushed a commit that referenced this pull request Oct 23, 2023
* Fix deprecated Numpy logic in `NormalizeRXAngles`

This new pass added in gh-10634 uses some deprecated Numpy properties
and has some slightly fragile exception-based logic when the required
properties can be directly tested.  This code issues warnings with Numpy
1.25+, which is currently not used by CI due to gh-10305.

* Fix return value

(cherry picked from commit 6bf90fa)
github-merge-queue bot pushed a commit that referenced this pull request Oct 23, 2023
* Fix deprecated Numpy logic in `NormalizeRXAngles`

This new pass added in gh-10634 uses some deprecated Numpy properties
and has some slightly fragile exception-based logic when the required
properties can be directly tested.  This code issues warnings with Numpy
1.25+, which is currently not used by CI due to gh-10305.

* Fix return value

(cherry picked from commit 6bf90fa)

Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
jakelishman added a commit to jakelishman/qiskit-terra that referenced this pull request Mar 12, 2024
This commit brings the Qiskit test suite to a passing state (with all
optionals installed) with Numpy 2.0.0b1, building on previous commits
that handled much of the rest of the changing requirements:

- Qiskitgh-10890
- Qiskitgh-10891
- Qiskitgh-10892
- Qiskitgh-10897
- Qiskitgh-11023

Notably, this commit did not actually require a rebuild of Qiskit,
despite us compiling against Numpy; it seems to happen that the C API
stuff we use via `rust-numpy` (which loads the Numpy C extensions
dynamically during module initialisation) hasn't changed.
jakelishman added a commit to jakelishman/qiskit-terra that referenced this pull request Apr 15, 2024
This commit brings the Qiskit test suite to a passing state (with all
optionals installed) with Numpy 2.0.0b1, building on previous commits
that handled much of the rest of the changing requirements:

- Qiskitgh-10890
- Qiskitgh-10891
- Qiskitgh-10892
- Qiskitgh-10897
- Qiskitgh-11023

Notably, this commit did not actually require a rebuild of Qiskit,
despite us compiling against Numpy; it seems to happen that the C API
stuff we use via `rust-numpy` (which loads the Numpy C extensions
dynamically during module initialisation) hasn't changed.

The main changes are:

- adapting to the changed `copy=None` and `copy=False` semantics in
  `array` and `asarray`.
- making sure all our implementers of `__array__` accept both `dtype`
  and `copy` arguments.

Co-authored-by: Lev S. Bishop <18673315+levbishop@users.noreply.github.com>
jakelishman added a commit to jakelishman/qiskit-terra that referenced this pull request Apr 16, 2024
This commit brings the Qiskit test suite to a passing state (with all
optionals installed) with Numpy 2.0.0b1, building on previous commits
that handled much of the rest of the changing requirements:

- Qiskitgh-10890
- Qiskitgh-10891
- Qiskitgh-10892
- Qiskitgh-10897
- Qiskitgh-11023

Notably, this commit did not actually require a rebuild of Qiskit,
despite us compiling against Numpy; it seems to happen that the C API
stuff we use via `rust-numpy` (which loads the Numpy C extensions
dynamically during module initialisation) hasn't changed.

The main changes are:

- adapting to the changed `copy=None` and `copy=False` semantics in
  `array` and `asarray`.
- making sure all our implementers of `__array__` accept both `dtype`
  and `copy` arguments.

Co-authored-by: Lev S. Bishop <18673315+levbishop@users.noreply.github.com>
github-merge-queue bot pushed a commit that referenced this pull request Apr 25, 2024
* Finalise support for Numpy 2.0

This commit brings the Qiskit test suite to a passing state (with all
optionals installed) with Numpy 2.0.0b1, building on previous commits
that handled much of the rest of the changing requirements:

- gh-10890
- gh-10891
- gh-10892
- gh-10897
- gh-11023

Notably, this commit did not actually require a rebuild of Qiskit,
despite us compiling against Numpy; it seems to happen that the C API
stuff we use via `rust-numpy` (which loads the Numpy C extensions
dynamically during module initialisation) hasn't changed.

The main changes are:

- adapting to the changed `copy=None` and `copy=False` semantics in
  `array` and `asarray`.
- making sure all our implementers of `__array__` accept both `dtype`
  and `copy` arguments.

Co-authored-by: Lev S. Bishop <18673315+levbishop@users.noreply.github.com>

* Update `__array__` methods for Numpy 2.0 compatibility

As of Numpy 2.0, implementers of `__array__` are expected and required
to have a signature

    def __array__(self, dtype=None, copy=None): ...

In Numpys before 2.0, the `copy` argument will never be passed, and the
expected signature was

    def __array__(self, dtype=None): ...

Because of this, we have latitude to set `copy` in our implementations
to anything we like if we're running against Numpy 1.x, but we should
default to `copy=None` if we're running against Numpy 2.0.

The semantics of the `copy` argument to `np.array` changed in Numpy 2.0.
Now, `copy=False` means "raise a `ValueError` if a copy is required" and
`copy=None` means "copy only if required".  In Numpy 1.x, `copy=False`
meant "copy only if required".  In _both_ Numpy 1.x and 2.0,
`ndarray.astype` takes a `copy` argument, and in both, `copy=False`
means "copy only if required".  In Numpy 2.0 only, `np.asarray` gained a
`copy` argument with the same semantics as the `np.array` copy argument
from Numpy 2.0.

Further, the semantics of the `__array__` method changed in Numpy 2.0,
particularly around copying.  Now, Numpy will assume that it can pass
`copy=True` and the implementer will handle this.  If `copy=False` is
given and a copy or calculation is required, then the implementer is
required to raise `ValueError`.  We have a few places where the
`__array__` method may (or always does) calculate the array, so in all
these, we must forbid `copy=False`.

With all this in mind: this PR sets up all our implementers of
`__array__` to either default to `copy=None` if they will never actually
need to _use_ the `copy` argument within themselves (except perhaps to
test if it was set by Numpy 2.0 to `False`, as Numpy 1.x will never set
it), or to a compatibility shim `_numpy_compat.COPY_ONLY_IF_NEEDED` if
they do naturally want to use it with those semantics.  The pattern

    def __array__(self, dtype=None, copy=_numpy_compat.COPY_ONLY_IF_NEEDED):
        dtype = self._array.dtype if dtype is None else dtype
        return np.array(self._array, dtype=dtype, copy=copy)

using `array` instead of `asarray` lets us achieve all the desired
behaviour between the interactions of `dtype` and `copy` in a way that
is compatible with both Numpy 1.x and 2.x.

* fixing numerical issues on mac-arm

* Change error to match Numpy

---------

Co-authored-by: Lev S. Bishop <18673315+levbishop@users.noreply.github.com>
Co-authored-by: Sebastian Brandhofer <148463728+sbrandhsn@users.noreply.github.com>
github-merge-queue bot pushed a commit that referenced this pull request Jun 18, 2024
* Finalise support for Numpy 2.0

This commit brings the Qiskit test suite to a passing state (with all
optionals installed) with Numpy 2.0.0b1, building on previous commits
that handled much of the rest of the changing requirements:

- gh-10890
- gh-10891
- gh-10892
- gh-10897
- gh-11023

Notably, this commit did not actually require a rebuild of Qiskit,
despite us compiling against Numpy; it seems to happen that the C API
stuff we use via `rust-numpy` (which loads the Numpy C extensions
dynamically during module initialisation) hasn't changed.

The main changes are:

- adapting to the changed `copy=None` and `copy=False` semantics in
  `array` and `asarray`.
- making sure all our implementers of `__array__` accept both `dtype`
  and `copy` arguments.

Co-authored-by: Lev S. Bishop <18673315+levbishop@users.noreply.github.com>

* Update `__array__` methods for Numpy 2.0 compatibility

As of Numpy 2.0, implementers of `__array__` are expected and required
to have a signature

    def __array__(self, dtype=None, copy=None): ...

In Numpys before 2.0, the `copy` argument will never be passed, and the
expected signature was

    def __array__(self, dtype=None): ...

Because of this, we have latitude to set `copy` in our implementations
to anything we like if we're running against Numpy 1.x, but we should
default to `copy=None` if we're running against Numpy 2.0.

The semantics of the `copy` argument to `np.array` changed in Numpy 2.0.
Now, `copy=False` means "raise a `ValueError` if a copy is required" and
`copy=None` means "copy only if required".  In Numpy 1.x, `copy=False`
meant "copy only if required".  In _both_ Numpy 1.x and 2.0,
`ndarray.astype` takes a `copy` argument, and in both, `copy=False`
means "copy only if required".  In Numpy 2.0 only, `np.asarray` gained a
`copy` argument with the same semantics as the `np.array` copy argument
from Numpy 2.0.

Further, the semantics of the `__array__` method changed in Numpy 2.0,
particularly around copying.  Now, Numpy will assume that it can pass
`copy=True` and the implementer will handle this.  If `copy=False` is
given and a copy or calculation is required, then the implementer is
required to raise `ValueError`.  We have a few places where the
`__array__` method may (or always does) calculate the array, so in all
these, we must forbid `copy=False`.

With all this in mind: this PR sets up all our implementers of
`__array__` to either default to `copy=None` if they will never actually
need to _use_ the `copy` argument within themselves (except perhaps to
test if it was set by Numpy 2.0 to `False`, as Numpy 1.x will never set
it), or to a compatibility shim `_numpy_compat.COPY_ONLY_IF_NEEDED` if
they do naturally want to use it with those semantics.  The pattern

    def __array__(self, dtype=None, copy=_numpy_compat.COPY_ONLY_IF_NEEDED):
        dtype = self._array.dtype if dtype is None else dtype
        return np.array(self._array, dtype=dtype, copy=copy)

using `array` instead of `asarray` lets us achieve all the desired
behaviour between the interactions of `dtype` and `copy` in a way that
is compatible with both Numpy 1.x and 2.x.

---------

Co-authored-by: Lev S. Bishop <18673315+levbishop@users.noreply.github.com>
Co-authored-by: Raynel Sanchez <87539502+raynelfss@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: None Do not include in changelog stable backport potential The bug might be minimal and/or import enough to be port to stable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants