Workflow : Add nightly release of NumPy in linux workflows by V0lantis · Pull Request #13876 · scipy/scipy

V0lantis · 2021-04-16T20:41:39Z

Reference issue

Since some of the PRs are relating on very recent NumPy's changes (as for #13833), we need to pull the NumPy Nightly Release when doing our workflows

What does this implement/fix?

This fetch the Nightly Numpy Release rather than the latest release of Numpy, in order to have the latest changes.
Right now, it it just implemented on the Linux's workflow, as I don't know if it is necessary to have it everywhere. If some more experienced reviewer believe we need it in every workflow, I'll change it right away.

andyfaff · 2021-04-16T21:47:34Z

I don't think we need it on the Python3.7-dbg, just the python 3.9.

peterbell10 · 2021-04-17T12:37:24Z

+REAL_DTYPES = Union[np.float32, np.float64, np.longdouble]
+COMPLEX_DTYPES = Union[np.complex64, np.complex128, np.clongdouble]
+DTYPES = Union[REAL_DTYPES, COMPLEX_DTYPES]


Suggested change

REAL_DTYPES = Union[np.float32, np.float64, np.longdouble]

COMPLEX_DTYPES = Union[np.complex64, np.complex128, np.clongdouble]

DTYPES = Union[REAL_DTYPES, COMPLEX_DTYPES]

REAL_DTYPES: List[type] = [np.float32, np.float64, np.longdouble]

COMPLEX_DTYPES: List[type] = [np.complex64, np.complex128, np.clongdouble]

DTYPES = REAL_DTYPES + COMPLEX_DTYPES

So what exactly is the goal of these constants?
Are they meant to represent type aliases or a sequence of types?

In the former case the original the original code was correct, in the latter I'd suggest using (unannotated) tuples rather than lists (as tuples can easily be used for expressing inhomogeneous sequences).

It's not a type annotation, it's a list of dtypes used in testing.

for dtype in REAL_DTYPES: # run test for dtype

Suggested change

REAL_DTYPES = Union[np.float32, np.float64, np.longdouble]

COMPLEX_DTYPES = Union[np.complex64, np.complex128, np.clongdouble]

DTYPES = Union[REAL_DTYPES, COMPLEX_DTYPES]

REAL_DTYPES = (np.float32, np.float64, np.longdouble)

COMPLEX_DTYPES = (np.complex64, np.complex128, np.clongdouble)

DTYPES = REAL_DTYPES + COMPLEX_DTYPES

Right, If so I'd suggest the changes below (non-explicitly annotated tuples rather than lists).

The advantage of using tuples here is that type checkers can link each DTYPES element to a particular np.inexact subclass, which is not possible with lists as they can only be annotated as homogeneous sequences.

from typing import TYPE_CHECKING import numpy as np REAL_DTYPES = (np.float32, np.float64, np.longdouble) COMPLEX_DTYPES = (np.complex64, np.complex128, np.clongdouble) DTYPES = REAL_DTYPES + COMPLEX_DTYPES if TYPE_CHECKING: # Revealed type is 'numpy.floating[numpy.typing._32Bit*]' reveal_type(REAL_DTYPES[0]()) # Revealed type is 'numpy.complexfloating[numpy.typing._32Bit*, numpy.typing._32Bit*]' reveal_type(COMPLEX_DTYPES[0]()) # Revealed type is 'numpy.complexfloating[numpy.typing._64Bit*, numpy.typing._64Bit*]' reveal_type(DTYPES[4]())

From the build :

scipy/linalg/tests/test_basic.py:32: note: Revealed type is 'numpy.floating[numpy.typing._32Bit*]' scipy/linalg/tests/test_basic.py:35: note: Revealed type is 'numpy.complexfloating[numpy.typing._32Bit*, numpy.typing._32Bit*]' scipy/linalg/tests/test_basic.py:38: note: Revealed type is 'numpy.complexfloating[numpy.typing._64Bit*, numpy.typing._64Bit*]'

I think I am not putting the condition snippet in the code, am I?

EDIT: commit the majority of it in d720c56

I think I am not putting the condition snippet in the code, am I?

Correct, this was merely intended as an example.

peterbell10 · 2021-04-17T12:41:49Z

+if TYPE_CHECKING:
+    Numbers = Union[Type[np.double], Type[np.float32], Type[int]]
+    DecMapType = Dict[
+        Tuple[
+            partial, Numbers, int
+        ],
+        int
+    ]


Suggested change

if TYPE_CHECKING:

Numbers = Union[Type[np.double], Type[np.float32], Type[int]]

DecMapType = Dict[

Tuple[

partial, Numbers, int

],

int

]

DecMapType = Dict[Tuple[Callable, type, int], int]

Don't forget the subscription.

EDIT: replace Union[Type[np.float32], Type[np.float64]] with Type[np.floating].

Suggested change

if TYPE_CHECKING:

Numbers = Union[Type[np.double], Type[np.float32], Type[int]]

DecMapType = Dict[

Tuple[

partial, Numbers, int

],

int

]

DecMapType = Dict[

# Or just `Type[np.floating]` instead of the union

Tuple[Callable[..., np.ndarray], Type[np.floating], int],

int,

]

I just tested with mypy and Callable with no arguments works exactly as expected.

Union[Type[np.float32], Type[np.float64]] is also wrong because we have tests for np.longdouble as well.

I just tested with mypy and Callable with no arguments works exactly as expected.

I'ts no suprise that that Callable without arguments works as expected, as it is equivalent to Callable[..., Any].

Union[Type[np.float32], Type[np.float64]] is also wrong because we have tests for np.longdouble as well.

In that case Type[np.floating] would definitely be more appropriate.

rgommers · 2021-04-17T12:53:08Z

    assert_array_almost_equal(x, y)


+if TYPE_CHECKING:


This line shouldn't be here. It causes DecMapType to not be defined at runtime, which causes the failures in CI:

NameError: name 'DecMapType' is not defined

In general you only want to use if TYPE_CHECKING is something does not work or is expensive at runtime.

ilayn · 2021-04-17T13:36:21Z

I don't see why we are touching the test files for the numpy workflow in this PR.

But in any case they are not meant for typing in the first place. These are meant to be looped over not for templating. Each dtype is being tested separately.

V0lantis · 2021-04-17T13:49:49Z

I don't see why we are touching the test files for the numpy workflow in this PR.

But in any case they are not meant for typing in the first place. These are meant to be looped over not for templating. Each dtype is being tested separately.

Because the changes in the workflows are breaking the type tests (python3.9 -u runtests.py --mypy) ... I don't see others solutions here, otherwise it will break all the following workflows following the merge of this branch. And this PR won't even be 'successful' over its own workflow

ilayn · 2021-04-17T14:01:45Z

This looks like a numpy/mypy issue instead of scipy to be honest. All that code is doing is putting dtype definitions into a list which is pretty straightforward and valid Python code.

What is the error that mypy is complaining about?

V0lantis · 2021-04-17T14:44:40Z

This looks like a numpy/mypy issue instead of scipy to be honest. All that code is doing is putting dtype definitions into a list which is pretty straightforward and valid Python code.

What is the error that mypy is complaining about?

Errors (https://github.com/scipy/scipy/runs/2365780037#step:6:24) were :

scipy/fft/_pocketfft/tests/test_real_transforms.py:256: error: Invalid index type "Tuple[partial[Any], Type[int], int]" for "Dict[Tuple[partial[Any], Type[floating[Any]], int], int]"; expected type "Tuple[partial[Any], Type[floating[Any]], int]"  [index]
22
scipy/linalg/tests/test_basic.py:26: error: Unsupported operand types for + ("List[Type[object]]" and "List[Type[object]]")  [operator]
23
Found 2 errors in 2 files (checked 679 source files)

ilayn · 2021-04-17T14:58:33Z

That sounds like mypy going awry. Lists in Python are by definition heterogeneous. But !, of course, typical typing hell is brought in by these

https://stackoverflow.com/questions/65361188/python-typing-concatenate-sequences
https://stackoverflow.com/questions/61861182/why-is-sequence-an-unsupported-operand-type-for-in-mypy

So the easiest is to spell out DTYPES explicitly and avoid the summation so that we don't need all that typing stuff.

V0lantis · 2021-04-20T10:04:23Z

That sounds like mypy going awry. Lists in Python are by definition heterogeneous. But !, of course, typical typing hell is brought in by these

https://stackoverflow.com/questions/65361188/python-typing-concatenate-sequences
https://stackoverflow.com/questions/61861182/why-is-sequence-an-unsupported-operand-type-for-in-mypy

So the easiest is to spell out DTYPES explicitly and avoid the summation so that we don't need all that typing stuff.

The changes proposed by @BvB93 seems to be working. Should I let it like that or implement your solution @ilayn ?
If not, the PR sounds ready to be merged for me :-)

ilayn · 2021-04-20T11:01:17Z

If it works, it works. so clicking the button. They are simple changes anyways hence we can revisit them if need be. Thanks all!

rgommers · 2021-04-20T13:43:54Z

This is failing: https://github.com/scipy/scipy/runs/2390972247?check_suite_focus=true

Note that changes to GitHub Actions files don't actually take effect until they are in master, for security reasons. The way to test this is to make the action run on your own fork, and push changes directly to master on your fork until the action passes.

ilayn · 2021-04-21T07:36:54Z

@V0lantis @BvB93 Are we fixing these in the other PR? Or should we revert it?

V0lantis · 2021-04-21T07:46:14Z

Sorry, I have been busy the past days 😕 I think revert and then I’ll submit an other PR to fix it ?

…

On Wed 21 Apr 2021 at 09:37, Ilhan Polat ***@***.***> wrote: @V0lantis <https://github.com/V0lantis> @BvB93 <https://github.com/BvB93> Are we fixing these in the other PR? Or should we revert it? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#13876 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AI7LNNRQFRKGWAVYU252QU3TJZ6CVANCNFSM43CGQPDA> .

This reverts commit 116f924.

…#13909) This reverts commit 116f924.

rgommers · 2021-04-21T08:06:49Z

Makes sense, reverted in gh-13909. Thanks @V0lantis

* master: (164 commits) DOC: Add Karl Pearson's reference to chi-square test (scipy#13971) BLD: fix build warnings for causal/anticausal pointers in ndimage MAINT: stats: Fix unused imports and a few other issues related to imports. DOC: fix typo MAINT: Remove duplicate calculations in sokalmichener BUG: spatial: fix weight handling of `distance.sokalmichener`. DOC: update Readme (scipy#13910) MAINT: QMCEngine d input validation (scipy#13940) MAINT: forward port 1.6.3 relnotes REL: add PEP 621 (project metadata in pyproject.toml) support EHN: signal: make `get_window` supports `general_cosine` and `general_hamming` window functions. (scipy#13934) ENH/DOC: pydata sphinx theme polishing (scipy#13814) DOC/MAINT: Add copyright notice to qmc.primes_from_2_to (scipy#13927) BUG: DOC: signal: fix need argument config and add missing doc link for `signal.get_window` DOC: fix subsets docstring (scipy#13926) BUG: signal: fix get_window argument handling and add tests. (scipy#13879) ENH: stats: add 'alternative' parameter to ansari (scipy#13650) BUG: Reactivate conda environment in init MAINT: use dict built-in rather than OrderedDict Revert "CI: Add nightly release of NumPy in linux workflows (scipy#13876)" (scipy#13909) ...

Add nightly release of numpy in linux workflows

344112f

tylerjereddy added the CI Items related to the CI tools such as CircleCI, GitHub Actions or Azure label Apr 16, 2021

V0lantis added 3 commits April 17, 2021 13:31

Remove NumPy Nightly Release from python3.7-dbg

b8fcaac

Fix mypy errors in test_basic.py

cb9f75a

Fix mypy errors in test_real_transforms.py

b807b2f

rgommers reviewed Apr 17, 2021

View reviewed changes

Comment thread .github/workflows/linux.yml Outdated

peterbell10 reviewed Apr 17, 2021

View reviewed changes

rgommers reviewed Apr 17, 2021

View reviewed changes

Remove one too much pytest-xdist

85a5d35

Update types' annotations

d720c56

ilayn mentioned this pull request Apr 17, 2021

MAINT: Add inline type hintings for stats.qmc #13833

Merged

ilayn merged commit 116f924 into scipy:master Apr 20, 2021

tylerjereddy added this to the 1.7.0 milestone Apr 21, 2021

rgommers added a commit that referenced this pull request Apr 21, 2021

Revert "CI: Add nightly release of NumPy in linux workflows (#13876)"

a912948

This reverts commit 116f924.

rgommers mentioned this pull request Apr 21, 2021

Revert "Workflow : Add nightly release of NumPy in linux workflows" #13909

Merged

rgommers added a commit that referenced this pull request Apr 21, 2021

Revert "CI: Add nightly release of NumPy in linux workflows (#13876)" (…

524b425

…#13909) This reverts commit 116f924.

V0lantis mentioned this pull request May 5, 2021

Workflow : Add nightly release of NumPy in linux workflows #13992

Merged

Uh oh!

Conversation

V0lantis commented Apr 16, 2021

Reference issue

What does this implement/fix?

Uh oh!

andyfaff commented Apr 16, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

V0lantis Apr 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BvB93 Apr 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ilayn commented Apr 17, 2021

Uh oh!

V0lantis commented Apr 17, 2021

Uh oh!

ilayn commented Apr 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

V0lantis commented Apr 17, 2021

Uh oh!

ilayn commented Apr 17, 2021

Uh oh!

V0lantis commented Apr 20, 2021

Uh oh!

ilayn commented Apr 20, 2021

Uh oh!

rgommers commented Apr 20, 2021

Uh oh!

ilayn commented Apr 21, 2021

Uh oh!

V0lantis commented Apr 21, 2021 via email

Uh oh!

rgommers commented Apr 21, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

V0lantis Apr 17, 2021 •

edited

Loading

BvB93 Apr 17, 2021 •

edited

Loading

ilayn commented Apr 17, 2021 •

edited

Loading