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

Fixes warning from setting categorical codes to floats #4624

Merged
merged 1 commit into from Mar 22, 2019

Conversation

Projects
None yet
3 participants
@jsignell
Copy link
Contributor

commented Mar 22, 2019

  • Fixes #4559
  • Tests added / passed - let me know if you think this needs one.
  • Passes flake8 dask
@martindurant

This comment has been minimized.

Copy link
Member

commented Mar 22, 2019

+1 , looks totally fine.
I don't know if there's a way to assert that a function call doesn't give a warning. If no one comments on here giving a way, I'll just merge in a few hours.

@jrbourbeau

This comment has been minimized.

Copy link
Member

commented Mar 22, 2019

pytest has a nice pytest.warns context manager that records raised warnings. Something like

with pytest.warns(None) as record:
    # code that doesn't raise a warning

assert len(record) == 0

could work here

@jsignell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2019

Yep that would work nicely, just not sure if it is overkill.

@martindurant

This comment has been minimized.

Copy link
Member

commented Mar 22, 2019

I am happy either way, up to you.

@jsignell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2019

I tried it out and it seems that it would fail on any FutureWarning which seems like something that dask would want to be intentional about enabling. So I think this should go in as-is.

RE #4559 (comment): I did a glance over unmerged PRs and didn't see this change in any of them.

@jrbourbeau

This comment has been minimized.

Copy link
Member

commented Mar 22, 2019

I think this should go in as-is

Agreed, failing on any warning here is probably overkill. I checked locally that the related FutureWarning is no longer raised. +1 from me

@martindurant martindurant merged commit eef368f into dask:master Mar 22, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jrbourbeau

This comment has been minimized.

Copy link
Member

commented Mar 22, 2019

Thanks @jsignell

asmith26 added a commit to asmith26/dask that referenced this pull request Apr 22, 2019

jorge-pessoa pushed a commit to jorge-pessoa/dask that referenced this pull request May 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.