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

Fix HDF4 reading utility using dtype classes instead of instances #891

Merged
merged 2 commits into from Aug 30, 2019

Conversation

djhoese
Copy link
Member

@djhoese djhoese commented Aug 30, 2019

I think we were getting lucky in previous versions of numpy and xarray where a dtype class could be used relatively interchangeably with a dytpe instance. Now in numpy 1.17 this isn't magic anymore and we have to fix some of our usage. For example, np.float32 is a class (a type) so things like np.float32.itemsize now return a property of a class not an instance.

This PR makes it so the HDF4 utility class uses instances of dtypes to avoid some errors I was getting.

  • Tests added and test suite added to parent suite
  • Tests passed
  • Passes flake8 satpy

@djhoese
Copy link
Member Author

djhoese commented Aug 30, 2019

I think the main issue here is with HDF4 (pyhdf's) object that we are adding a dtype to. Numpy is still fine with np.array([1, 2, 3], dtype=np.float32) and doing my_arr.dtype.itemsize works. I can't seem to get a combination of dask and numpy to not work with np.float32 style usage.

Copy link
Member

@mraspaud mraspaud left a comment

Choose a reason for hiding this comment

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

Good catch ! LGTM

@djhoese
Copy link
Member Author

djhoese commented Aug 30, 2019

So I did a little more investigating. The var in from_sds is a pyhdf SDS object. We add a dtype and a shape and then dask seems to be able to treat it like a numpy array. One odd thing is that dask normally looks for an ndim attribute on objects, but since our shape is a list it doesn't need it. If I make shape a tuple then it complains about not having ndim. I...don't know. Going to leave it since it works and when it breaks we can look in to it more.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.003%) to 84.641% when pulling 48591e5 on djhoese:bugfix-hdf4-dtype into b6e37ce on pytroll:master.

@codecov
Copy link

codecov bot commented Aug 30, 2019

Codecov Report

Merging #891 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #891      +/-   ##
==========================================
+ Coverage   84.64%   84.65%   +<.01%     
==========================================
  Files         171      171              
  Lines       25033    25034       +1     
==========================================
+ Hits        21189    21192       +3     
+ Misses       3844     3842       -2
Impacted Files Coverage Δ
satpy/readers/hdf4_utils.py 92.72% <100%> (ø) ⬆️
satpy/tests/reader_tests/test_hdf4_utils.py 92.18% <100%> (+0.12%) ⬆️
satpy/readers/mersi2_l1b.py 97.59% <0%> (+1.2%) ⬆️
satpy/readers/virr_l1b.py 100% <0%> (+1.58%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b6e37ce...48591e5. Read the comment docs.

@djhoese djhoese merged commit ebede6d into pytroll:master Aug 30, 2019
@djhoese djhoese deleted the bugfix-hdf4-dtype branch August 30, 2019 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants