-
Notifications
You must be signed in to change notification settings - Fork 1.1k
lookup_linke_turbidity speed improvement, mat to h5 #442
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
Conversation
* replace LinkeTurbidity matlab file by hdf5 (gzip 4) * update for using LinkeTurbidity hdf5 instead of matlab file in lookup_linke_turbidity function * tests dependencies updates * update requirements / ci for LinkeTurbidity hdf5 file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! Just a few changes needed.
appveyor.yml
Outdated
@@ -28,7 +28,7 @@ install: | |||
- cmd: conda info -a | |||
|
|||
# install depenencies | |||
- cmd: conda create -n test_env --yes --quiet python=%PYTHON_VERSION% pip numpy scipy pandas nose pytest pytz ephem numba siphon -c conda-forge | |||
- cmd: conda create -n test_env --yes --quiet python=%PYTHON_VERSION% pip numpy scipy tables pandas nose pytest pytz ephem numba siphon -c conda-forge |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you want pytables
here and in the env files below.
@@ -9,7 +9,7 @@ API Changes | |||
|
|||
Enhancements | |||
~~~~~~~~~~~~ | |||
* | |||
* Improve clearsky.lookup_linke_turbidity speed (IO) (:issue:`437`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add the .mat
to .h5
change to the API changes section.
try: | ||
lts = lt_h5_file.root.LinkeTurbidity[latitude_index, longitude_index, :] | ||
except IndexError: | ||
raise IndexError('Latitude should be between 90 and -90, ' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add a test that asserts that this exception is raised under invalid inputs.
Also, I confirmed that the code below does in fact execute the finally block despite a new exception being raised.
try:
raise IndexError
except IndexError:
raise IndexError('blah')
finally:
print('finally')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually did write a test for that, but then realized it was already covered.
For the try except finally
, I was hoping open_file
to come with a context manager, but it does not look like it is the case and fall back to the old school way to make sure the file get closed whatever happen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my bad, thanks for reminding me...
Looks like the build is passing now. I can squash commits if needed. |
Looks finished to me. I plan to merge tomorrow, so now is the time to comment if anyone has objections. |
git diff upstream/master -u -- "*.py" | flake8 --diff
and/or landscape.io linting service.docs/sphinx/source/api.rst
for API changes.docs/sphinx/source/whatsnew
file for all changes.Note:
git diff upstream/master -u -- "*.py" | flake8 --diff
is giving mefatal: bad revision 'upstream/master'
. I usepylint
though and it didn't complain on additions.