roocs / clisops

Climate Simulation Operations
https://clisops.readthedocs.io/en/latest/
Other
21 stars 9 forks source link

update subset-by-point branch #192

Closed cehbrecht closed 3 years ago

cehbrecht commented 3 years ago

This PR updates the subset-by-point branch.

Changes:

agstephens commented 3 years ago

@cehbrecht this looks fine to me.

Zeitsperre commented 3 years ago

I think pre-commit needs to be updated and re-run over the library. If needed, I can do that quickly and add that here or on a different PR.

agstephens commented 3 years ago

I think pre-commit needs to be updated and re-run over the library. If needed, I can do that quickly and add that here or on a different PR.

Thanks @Zeitsperre, we'll do that 👍

cehbrecht commented 3 years ago

@agstephens @Zeitsperre We may ignore this branch ... it is still on draft. We can do the clean up (black ...) on the subset-by-point branch. I would walk through the dependency chain ... starting with a release of roocs-utils.

cehbrecht commented 3 years ago

@agstephens There is an issue with this code on python 3.6: https://github.com/roocs/clisops/blob/bfa838fd064f573da44bce238bb95bc414575f90/clisops/core/subset.py#L329-L330

On python > 3.6 tm_class is cftime._cftime.DatetimeGregorian, but on Python 3.6 it is numpy.datetime64. We can fix this on the subset-by-point branch.

cehbrecht commented 3 years ago

@agstephens There is an issue with this code on python 3.6:

https://github.com/roocs/clisops/blob/bfa838fd064f573da44bce238bb95bc414575f90/clisops/core/subset.py#L329-L330

On python > 3.6 tm_class is cftime._cftime.DatetimeGregorian, but on Python 3.6 it is numpy.datetime64. We can fix this on the subset-by-point branch.

@Zeitsperre I talked to Ag about this issue. The easiest fix for us would be to skip support for Python 3.6. Would this be ok on your side? Otherwise I would handle the py36 case for the numpy.datetime64 differently in the code.

Zeitsperre commented 3 years ago

@Zeitsperre I talked to Ag about this issue. The easiest fix for us would be to skip support for Python 3.6. Would this be ok on your side? Otherwise I would handle the py36 case for the numpy.datetime64 differently in the code.

Perfectly fine! We dropped support for Python3.6 in xclim more than 6 months ago. No issue on our end if it makes more sense to drop it. Thanks for checking!

cehbrecht commented 3 years ago

@ellesmith88 Please, just have a slight look at this PR :) I will merge then to subset-by-point branch and open a PR for this one. We can make further (minor) changes on the subset-by-point directly.

ellesmith88 commented 3 years ago

@cehbrecht I think it looks fine. For the release PR I will put roocs-utils>=0.5.0 into requirements.txt and environment.yml.

I was wondering why the xarray ci run has been removed? and also, is it ok for the xesmf tests to fail?

cehbrecht commented 3 years ago

@cehbrecht I think it looks fine. For the release PR I will put roocs-utils>=0.5.0 into requirements.txt and environment.yml.

ok 👍

I was wondering why the xarray ci run has been removed? and also, is it ok for the xesmf tests to fail?

I have cleaned up the tox.ini ...there was no testenv defined for py37-xarray. Probably it was meant to check against xarray master version. I allow xexmf to fail ... it is used for the regridding ... another story and not needed for this release.

ellesmith88 commented 3 years ago

@cehbrecht haha ok! Fine by me