Skip to content

Conversation

@ocefpaf
Copy link
Member

@ocefpaf ocefpaf commented Feb 23, 2018

This PR uses the new netcdftime package instead of netCDF4-python.

I can rebase this later but I left the individual commits to make it easier to review.

There are 6 tests failing but I believe they are unrelated to this PR b/c they are failing on master too. Note that on master py34 was passing, but I guess that is due to an old netCDF4-python version.

.travis.yml Outdated

python:
- 2.7
- 3.4
Copy link
Member Author

Choose a reason for hiding this comment

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

conda-forge deprecated py34

- conda config --add channels conda-forge
- ENV_NAME='test-environment'
- conda create --quiet -n $ENV_NAME python=$TRAVIS_PYTHON_VERSION
- conda create --name $ENV_NAME python=$TRAVIS_PYTHON_VERSION udunits2 --file requirements.txt --file requirements-dev.txt
Copy link
Member Author

Choose a reason for hiding this comment

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

Building the env in one go saves us some trouble. Sometimes conda does not respect channel preference when adding packages to an env.

setup.py Outdated
import versioneer


class PyTest(TestCommand):
Copy link
Member Author

Choose a reason for hiding this comment

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

I could not get this to work, kept getting "tests not found." I switched to a direct pytest call with the pytest-cov plugin instead, hope that is OK.

@pelson
Copy link
Member

pelson commented Apr 30, 2018

Looks like some genuine test failures here. Happy in general with the changes. Thanks @ocefpaf!

@ocefpaf
Copy link
Member Author

ocefpaf commented Apr 30, 2018

Looks like some genuine test failures here.

I'll take a closer look at those soon.

Happy in general with the changes.

Maybe we should wait for a stable release of cftime and/or the next release of netCDF4, which will be using that. The package is not stable yet and they just renamed it.

@ocefpaf
Copy link
Member Author

ocefpaf commented Apr 30, 2018

@pelson the failures seems to be due to a change in how the comparison works. It is refusing compare dates with different calendars:

TypeError: cannot compare cftime._cftime.Datetime360Day(1970, 1, 1, 0, 0, 20, 0, -1, 1) and cftime._cftime.datetime(1970, 1, 1, 0, 0, 20, 0, -1, 1) (different calendars)

That makes sense to me but I'm not sure how to proceed to make the tests valid again. Maybe we can compare the timetuple like

cftime.datetime(1970, 1, 1, 0, 0, 20).timetuple() == _num2date_to_nearest_second(20., useconds).timetuple()
True

Is that OK?

@pelson
Copy link
Member

pelson commented May 2, 2018

Maybe we should wait for a stable release of cftime and/or the next release of netCDF4, which will be using that. The package is not stable yet and they just renamed it.

That works for me.

the failures seems to be due to a change in how the comparison works. It is refusing compare dates with different calendars:

In which case, cf_units is testing questionable behaviour IMO. I'm fully supportive of removing any such behaviour, unless there was a genuine mistake in writing the test (I didn't look yet).

- conda config --set show_channel_urls True
- conda config --set always_yes yes --set changeps1 no --set show_channel_urls True
- conda update --quiet conda
- conda config --add channels conda-forge
Copy link
Member Author

Choose a reason for hiding this comment

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

conda-forge's udunits2 is newer, hope that this change is OK @bjlittle

@coveralls
Copy link

coveralls commented May 2, 2018

Coverage Status

Coverage remained the same at 87.819% when pulling 7751642 on ocefpaf:use_netcdftime into d6d53fd on SciTools:master.

@bjlittle bjlittle mentioned this pull request May 2, 2018
@bjlittle
Copy link
Member

bjlittle commented May 2, 2018

@ocefpaf Could you rebase, now that #91 has been merged, thanks!

@ocefpaf
Copy link
Member Author

ocefpaf commented May 2, 2018

@ocefpaf Could you rebase, now that #91 has been merged, thanks!

Already did 😄

@bjlittle bjlittle self-assigned this May 2, 2018
@bjlittle
Copy link
Member

bjlittle commented May 2, 2018

@ocefpaf The coveralls coverage has tanked... perhaps due to the introduction of cftime?

Could you update the .coveragerc file to also omit the .eggs directory i.e.

[run]
branch = True
omit =
    setup.py
    versioneer.py
    cf_units/_version.py
    cf_units/etc/*
    cf_units/tests/*
    .eggs

That should do the trick...

@bjlittle
Copy link
Member

bjlittle commented May 2, 2018

@ocefpaf Let's do that rebase tango again... 😉

@ocefpaf
Copy link
Member Author

ocefpaf commented May 2, 2018

@ocefpaf Let's do that rebase tango again... 😉

Done!

@bjlittle
Copy link
Member

bjlittle commented May 2, 2018

@ocefpaf Awesome, thanks 👍

@bjlittle bjlittle merged commit c5ca970 into SciTools:master May 2, 2018
@ocefpaf ocefpaf deleted the use_netcdftime branch May 2, 2018 15:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants