Skip to content
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

Add test for NEP-18. #4675

Merged
merged 1 commit into from Apr 8, 2019
Merged

Add test for NEP-18. #4675

merged 1 commit into from Apr 8, 2019

Conversation

@hameerabbasi
Copy link
Contributor

@hameerabbasi hameerabbasi commented Apr 8, 2019

  • Tests added / passed
  • Passes flake8 dask

Adds a simple test to check if NEP-18 is active.

try:
return np.concatenate([A()])
except ValueError:
return False
Copy link
Member

@mrocklin mrocklin Apr 8, 2019

Nice check :)

Copy link
Contributor Author

@hameerabbasi hameerabbasi Apr 8, 2019

My thought was, "how do I test the protocol, not an arbitrary-ish condition".

@hameerabbasi
Copy link
Contributor Author

@hameerabbasi hameerabbasi commented Apr 8, 2019

All green here... Any other changes needed?

@hameerabbasi
Copy link
Contributor Author

@hameerabbasi hameerabbasi commented Apr 8, 2019

(It seems from codecov that array function is never tested, at all, and locally two tests fail with array function on, even though nothing was touched)

@mrocklin
Copy link
Member

@mrocklin mrocklin commented Apr 8, 2019

It seems from codecov that array function is never tested

Perhaps it's only showing you information from one of the testing matrix elements?

and locally two tests fail with array function on

Which tests?

@hameerabbasi
Copy link
Contributor Author

@hameerabbasi hameerabbasi commented Apr 8, 2019

Which tests?

The two FFT ones. It was weird to diagnose because all of them are listed as lambdas.

Perhaps it's only showing you information from one of the testing matrix elements?

I highly doubt it... There's this line in .travis.yml:

- codecov

Edit: And I know codecov aggregates.

@hameerabbasi
Copy link
Contributor Author

@hameerabbasi hameerabbasi commented Apr 8, 2019

cc @pentschev Maybe you can provide some input here. :-)

@pentschev
Copy link
Member

@pentschev pentschev commented Apr 8, 2019

The FFT tests depend NumPy compiled without MKL, Dask won't dispatch otherwise. There's an issue open for that #3281.

@pentschev
Copy link
Member

@pentschev pentschev commented Apr 8, 2019

Btw, I have a workaround for the FFT tests 8f33dcc, but I can't get that PR merged.

@pentschev
Copy link
Member

@pentschev pentschev commented Apr 8, 2019

Actually, this is why I hate stacking fixes in the same PR, it gets more difficult to get them merged and then people start running into problems that could have been already solved otherwise.

@hameerabbasi
Copy link
Contributor Author

@hameerabbasi hameerabbasi commented Apr 8, 2019

So, this one should be good to go then, I guess. :-)

@mrocklin mrocklin merged commit 8e757ef into dask:master Apr 8, 2019
3 checks passed
@mrocklin
Copy link
Member

@mrocklin mrocklin commented Apr 8, 2019

Thanks @hameerabbasi . This is in

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants