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.

Sign up for GitHub

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

Jump to bottom

BUG: fix safe_cast_to_index #3001

Merged
merged 1 commit into from
Jun 10, 2019
Merged

BUG: fix safe_cast_to_index #3001

merged 1 commit into from
Jun 10, 2019

Conversation

0x0L
Copy link
Contributor

@0x0L 0x0L commented Jun 5, 2019 •
edited
Loading

@pep8speaks
Copy link

pep8speaks commented Jun 5, 2019 •
edited
Loading

Hello @0x0L! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-06-06 18:11:58 UTC

@0x0L 0x0L force-pushed the fix0 branch 2 times, most recently from e628db6 to 7644c06 Compare June 5, 2019 23:08
@0x0L 0x0L changed the title fix safe_cast_to_index BUG: fix safe_cast_to_index Jun 5, 2019
spencerkclark
spencerkclark reviewed Jun 6, 2019
View reviewed changes
Copy link
Member

@spencerkclark spencerkclark left a comment

Choose a reason for hiding this comment

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

Thanks for catching this and sending a fix @0x0L!

doc/whats-new.rst Outdated Show resolved Hide resolved
xarray/core/utils.py Show resolved Hide resolved
xarray/core/utils.py Outdated Show resolved Hide resolved
@0x0L
Copy link
Contributor Author

0x0L commented Jun 6, 2019

@spencerkclark
Are you ok with keeping the len > 0 test and then just moving up the assert at the top of the constructor of CFTimeIndex so that we raise before allocating the memory with result._data = np.array(data, dtype='O')

@spencerkclark
Copy link
Member

Are you ok with keeping the len > 0 test and then just moving up the assert at the top of the constructor of CFTimeIndex so that we raise before allocating the memory with result._data = np.array(data, dtype='O')

Yes, sorry, it seems I'm a bit sleepy today as well. This is probably the best approach 👍.

spencerkclark
spencerkclark approved these changes Jun 7, 2019
View reviewed changes
Copy link
Member

@spencerkclark spencerkclark left a comment

Choose a reason for hiding this comment

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

This looks good to me; thanks again for your patience @0x0L.

@shoyer shoyer merged commit 7e649e4 into pydata:master Jun 10, 2019
dcherian added a commit to dcherian/xarray that referenced this pull request Jun 24, 2019
@dcherian
Merge branch 'master' into fix/bounds_encode_2
f187ca1 
* master: (31 commits)
  Add quantile method to GroupBy (pydata#2828)
  rolling_exp (nee ewm) (pydata#2650)
  Ensure explicitly indexed arrays are preserved (pydata#3027)
  add back dask-dev tests (pydata#3025)
  ENH: keepdims=True for xarray reductions (pydata#3033)
  Revert cmap fix (pydata#3038)
  Add "errors" keyword argument to drop() and drop_dims() (pydata#2994) (pydata#3028)
  More consistency checks (pydata#2859)
  Check types in travis (pydata#3024)
  Update issue templates (pydata#3019)
  Add pytest markers to avoid warnings (pydata#3023)
  Feature/merge errormsg (pydata#2971)
  More support for missing_value. (pydata#2973)
  Use flake8 rather than pycodestyle (pydata#3010)
  Pandas labels deprecation (pydata#3016)
  Pytest capture uses match, not message (pydata#3011)
  dask-dev tests to allowed failures in travis (pydata#3014)
  Fix 'to_masked_array' computing dask arrays twice (pydata#3006)
  str accessor (pydata#2991)
  fix safe_cast_to_index (pydata#3001)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spencerkclark spencerkclark spencerkclark approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Slowness when cftime is installed
4 participants
@0x0L @pep8speaks @spencerkclark @shoyer