search

USDA-ARS-NWRC / awsm

Automated Water Supply Model (AWSM) was developed at the USDA Agricultural Research Service. AWSM was designed to streamline the workflow used to forecast the water supply of multiple water basins.
Other
9 stars 2 forks source link

Require cftime to version greater than 1.1.0 #65

Closed jomey closed 4 years ago

jomey commented 4 years ago

Wasn't too big of a change to make AWSM work with the latest cftime. I checked SMRF and weather_forecast_retrieval as well and couldn't find a similar conversion. Hence I decided to go for this.

Snuck in a small clean up for awsm_daily_airflow and removed unused imports and separated run logic from argument parsing.

jomey commented 4 years ago

Documenting some related issues and references that helped for this issue: USDA-ARS-NWRC/basin_setup/issues/49 https://pypi.org/project/cftime/ version changes to 1.1.0 on 2/12

micahjohnson150 commented 4 years ago

Thanks for putting in the work to update this. What do we think about hard pinning the cftime? I don't have a version in mind but I would like to remove the possibility of the "surprise stuff is broken" situation again.

jomey commented 4 years ago

Thanks for putting in the work to update this. What do we think about hard pinning the cftime? I don't have a version in mind but I would like to remove the possibility of the "surprise stuff is broken" situation again.

How about we pin cftime to the current version? We are only using it for a simply date conversion to be timezone aware.

I wish the netCDF would do a better job of declaring their dependencies and want of such a change.

jomey commented 4 years ago

Maybe I am missing it but I don't see where we pinned cftime or even made it > 1.1.0. Was it introduced in another PR or something?

Instead of pinning the version, I decided to explicitly pass in the two parameters:

This is basically what is required to set timezones on the data type returned from num2date

micahjohnson150 commented 4 years ago

Gotcha, I do like the verbosity of that. I still think it would be nice to pin it either here or in SMRF, honestly probably should be SMRF. The num2date change was unexpected and ultimately a surprise so I just want to reduce that possibility again.

jomey commented 4 years ago

I will add it to here, since the call is made within the framework of AWSM

jomey commented 4 years ago

Pinned the version to 1.1.2, since there was a bug for these two parameters and got fixed in this version. I actually run into that and never got around to report it - glad someone did.

PR on cftime