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

Conditional import of the DatetimeAccessor due to xarray update #1078

Merged
merged 1 commit into from Jul 3, 2019

Conversation

zbruick
Copy link
Contributor

@zbruick zbruick commented Jul 1, 2019

Xarray v0.12.2 changed how it handles the DatetimeAccessor. As a result, a conditional import is needed to allow for Python 2 support. This can be updated back to a simple import of the v0.12.2 version once Python 2 support is dropped.

Fixes #1077.

@jthielen
Copy link
Collaborator

jthielen commented Jul 2, 2019

Since the DatetimeAccessor is only imported to add strftime(), would it make sense to remove the imports from the top of the file, and instead change the patch to something like the following?

# If DatetimeAccessor does not have a strftime (xarray <0.12.2), monkey patch one in
try:
    from xarray.core.accessors import DatetimeAccessor
    if not hasattr(DatetimeAccessor, 'strftime'):
        def strftime(self, date_format):
            """Format time as a string."""
            import pandas as pd
            values = self._obj.data
            values_as_series = pd.Series(values.ravel())
            strs = values_as_series.dt.strftime(date_format)
            return strs.values.reshape(values.shape)

        DatetimeAccessor.strftime = strftime
except ImportError:
    pass

This would also remove the Python 2.7 problem with importlib.util and only import from xarray.core where necessary.

@zbruick
Copy link
Contributor Author

zbruick commented Jul 2, 2019

Should we just vendor this as well and include it in #1079 for sake of consistency? Seems like that would be better than having multiple methods for importing things from xarray.core.

I didn't realize there were issues with importlib.util - I haven't through about 2.7 in so long.

@jthielen
Copy link
Collaborator

jthielen commented Jul 2, 2019

I could be mistaken, but I don't think we can vendor in DatetimeAccessor, since this has to modify the actual accessor that xarray uses internally in order to add .strftime() to the existing da.dt.

@zbruick
Copy link
Contributor Author

zbruick commented Jul 2, 2019

Gotcha - that makes sense. Glad you know your way around this code to help me. I'll make the changes you suggested and push it up.

metpy/xarray.py Outdated Show resolved Hide resolved
backend API of xarray v0.12.2. Fixes Unidata#1077.

Updated import method per @jthielen's suggestion
Copy link
Member

@dopplershift dopplershift 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 knocking out a fix so quickly.

@dopplershift dopplershift added Area: Xarray Pertains to xarray integration Type: Maintenance Updates and clean ups (but not wrong) labels Jul 2, 2019
@dopplershift dopplershift added this to the 0.11 milestone Jul 2, 2019
@dopplershift
Copy link
Member

Failures are just SkewT. Merging...

@dopplershift dopplershift merged commit ae0c67d into Unidata:master Jul 3, 2019
@dopplershift dopplershift modified the milestones: 0.11, 0.10.1 Jul 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Xarray Pertains to xarray integration Type: Maintenance Updates and clean ups (but not wrong)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Xarray v0.12.2 (29 June 2019) breaking metpy No module named 'xarray.core.accessors'
3 participants