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

sel with categorical index #3670

Merged
merged 12 commits into from
Jan 25, 2020
Merged

sel with categorical index #3670

merged 12 commits into from
Jan 25, 2020

Conversation

fujiisoup
Copy link
Member

@fujiisoup fujiisoup commented Jan 8, 2020 •
edited
Loading

It is a bit surprising that no members have used xarray with CategoricalIndex...
If there is anything missing additionally, please feel free to point it out.

@fujiisoup
Copy link
Member Author

I don't think the check failure is related.

@fujiisoup fujiisoup mentioned this pull request Jan 8, 2020
Closed
This was referenced Jan 9, 2020
Open
Closed
@pep8speaks
Copy link

pep8speaks commented Jan 11, 2020 •
edited
Loading

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

Line 13:4: W291 trailing whitespace
Line 14:85: W291 trailing whitespace

Comment last updated at 2020-01-25 00:59:20 UTC

@fujiisoup
Copy link
Member Author

I think this PR is ready for review.

@fujiisoup
Copy link
Member Author

I'll merge this tomorrow if no more commens.

keewis
keewis reviewed Jan 15, 2020
View reviewed changes
Copy link
Collaborator

@keewis keewis left a comment

Choose a reason for hiding this comment

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

I tried to look over the code but didn't find anything except some stylistic issues which I don't feel strongly about. Other than that this looks good to me (I don't know much about this part of xarray, though).

xarray/core/indexes.py
Comment on lines +19 to +23
levels = []
for i, level in enumerate(index.levels):
if isinstance(level, pd.CategoricalIndex):
level = level[index.codes[i]].remove_unused_categories()
levels.append(level)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel like this could be cleaner by using a list comprehension:

levels = [
    level[index.codes[i]].remove_unused_categories()
    if isinstance(level, pd.CategoricalIndex)
    else level
    for i, level in enumerate(index.levels)
]

though that might be just me

xarray/core/indexes.py Outdated Show resolved Hide resolved
dcherian
dcherian approved these changes Jan 16, 2020
View reviewed changes
Copy link
Contributor

@dcherian dcherian left a comment

Choose a reason for hiding this comment

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

Some minor comments. Thanks @fujiisoup

xarray/core/dataset.py Outdated Show resolved Hide resolved
xarray/core/indexes.py Outdated Show resolved Hide resolved
xarray/core/indexing.py Outdated Show resolved Hide resolved
xarray/core/indexing.py Outdated Show resolved Hide resolved
dcherian
dcherian reviewed Jan 16, 2020
View reviewed changes
doc/whats-new.rst Outdated Show resolved Hide resolved
@dcherian dcherian mentioned this pull request Jan 17, 2020
Closed
11 tasks
@keewis
Copy link
Collaborator

keewis commented Jan 25, 2020

unless I'm missing something this should be really close (run black once and fix the merge conflict in whats-new.rst)

dcherian and others added 3 commits January 24, 2020 17:58
@dcherian
blacken
5cdf82e
@dcherian
Merge branch 'master' of github.com:pydata/xarray into sel_categorilical
f259abc 
* 'master' of github.com:pydata/xarray: (27 commits)
  bump min deps for 0.15 (pydata#3713)
  setuptools-scm and isort tweaks (pydata#3720)
  Allow binned coordinates on 1D plots y-axis. (pydata#3685)
  apply_ufunc: Add meta kwarg + bump dask to 2.2 (pydata#3660)
  setuptools-scm and one-liner setup.py (pydata#3714)
  Feature/align in dot (pydata#3699)
  ENH: enable `H5NetCDFStore` to work with already open h5netcdf.File a… (pydata#3618)
  One-off isort run (pydata#3705)
  hardcoded xarray.__all__ (pydata#3703)
  Bump mypy to v0.761 (pydata#3704)
  remove DataArray and Dataset constructor deprecations for 0.15  (pydata#3560)
  Tests for variables with units (pydata#3654)
  Add an example notebook using apply_ufunc to vectorize 1D functions (pydata#3629)
  Use encoding['dtype'] over data.dtype when possible within CFMaskCoder.encode (pydata#3652)
  allow passing any iterable to drop when dropping variables (pydata#3693)
  Typo on DataSet/DataArray.to_dict documentation (pydata#3692)
  Fix mypy type checking tests failure in ds.merge (pydata#3690)
  Explicitly convert result of pd.to_datetime to a timezone-naive type (pydata#3688)
  ds.merge(da) bugfix (pydata#3677)
  fix docstring for combine_first: returns a Dataset (pydata#3683)
  ...
@keewis
delete trailing whitespace
27f3505
@keewis
Copy link
Collaborator

keewis commented Jan 25, 2020

should be ready for merging now

@fujiisoup
Copy link
Member Author

Thanks, @dcherian and @keewis , for keeping this updated.
Merging.

@fujiisoup fujiisoup merged commit cc142f4 into pydata:master Jan 25, 2020
@fujiisoup fujiisoup deleted the sel_categorilical branch January 25, 2020 22:38
dcherian added a commit to dcherian/xarray that referenced this pull request Jan 27, 2020
@dcherian
Merge branch 'master' into groupby-plot
e694a50 
* master:
  Add support for CFTimeIndex in get_clean_interp_index (pydata#3631)
  sel with categorical index (pydata#3670)
  bump min deps for 0.15 (pydata#3713)
  setuptools-scm and isort tweaks (pydata#3720)
  Allow binned coordinates on 1D plots y-axis. (pydata#3685)
  apply_ufunc: Add meta kwarg + bump dask to 2.2 (pydata#3660)
  setuptools-scm and one-liner setup.py (pydata#3714)
  Feature/align in dot (pydata#3699)
  ENH: enable `H5NetCDFStore` to work with already open h5netcdf.File a… (pydata#3618)
  One-off isort run (pydata#3705)
  hardcoded xarray.__all__ (pydata#3703)
  Bump mypy to v0.761 (pydata#3704)
  remove DataArray and Dataset constructor deprecations for 0.15  (pydata#3560)
  Tests for variables with units (pydata#3654)
  Add an example notebook using apply_ufunc to vectorize 1D functions (pydata#3629)
  Use encoding['dtype'] over data.dtype when possible within CFMaskCoder.encode (pydata#3652)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dcherian dcherian dcherian approved these changes

@keewis keewis keewis left review comments

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

Successfully merging this pull request may close these issues.

Fail to sel() when index comes from categorical pandas Series
4 participants
@fujiisoup @pep8speaks @keewis @dcherian