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

Update omps edr reader and hdf5_utils to handle OMPS SO2 data from FMI #1126

Merged

Conversation

TAlonglong
Copy link
Collaborator

@TAlonglong TAlonglong commented Mar 31, 2020

Update OMPS edr reader and hdf5_utils to handle OMPS SO2 DATA from FMI SAMPO.

The OMPS SO2 data from FMI SAMPO in hdf5 format contains references that need special handling. This update does this.

Also added dataset names special to the data from FMI SAMPO

@ghost
Copy link

ghost commented Mar 31, 2020

Congratulations 🎉. DeepCode analyzed your code in 1.109 seconds and we found no issues. Enjoy a moment of no bugs ☀️.

👉 View analysis in DeepCode’s Dashboard

@TAlonglong
Copy link
Collaborator Author

Added updates after major help from @mraspaud, @pnuu and Timo

@mraspaud mraspaud added component:readers enhancement code enhancements, features, improvements labels Mar 31, 2020
@djhoese
Copy link
Member

djhoese commented Mar 31, 2020

This is related to #883 and #630 I think. @TAlonglong thanks for working on this. Any idea if this fixes things for these issues?

@TAlonglong
Copy link
Collaborator Author

Yes @djhoese both of them. One of them is mine. Forgot I issued that :-)

Copy link
Member

@djhoese djhoese left a comment

Choose a reason for hiding this comment

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

Thanks again for this. I had a small suggestion on the name of the lon/lat datasets. Could you also add a test to the tests for the HDF5 utils module?

satpy/etc/readers/omps_edr.yaml Outdated Show resolved Hide resolved
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.

LGTM, just some consistency casing remarks

satpy/etc/readers/omps_edr.yaml Outdated Show resolved Hide resolved
resolution: 50000
coordinates: [Longitude, Latitude]
file_type: omps_sampo
file_key: SCIENCE_DATA/CloudFraction
Copy link
Member

Choose a reason for hiding this comment

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

Also about consistency, should the dataset have same case and wording as the ones from the other files ?

@coveralls
Copy link

coveralls commented Mar 31, 2020

Coverage Status

Coverage increased (+0.08%) to 89.605% when pulling 4820890 on TAlonglong:feature-omps-reader-update-for-sampo into 103d380 on pytroll:master.

@codecov
Copy link

codecov bot commented Mar 31, 2020

Codecov Report

Merging #1126 into master will increase coverage by 0.08%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1126      +/-   ##
==========================================
+ Coverage   89.52%   89.60%   +0.08%     
==========================================
  Files         200      200              
  Lines       29379    29484     +105     
==========================================
+ Hits        26301    26420     +119     
+ Misses       3078     3064      -14     
Impacted Files Coverage Δ
satpy/readers/hdf5_utils.py 92.42% <100.00%> (+0.62%) ⬆️
satpy/readers/omps_edr.py 100.00% <100.00%> (+3.84%) ⬆️
satpy/tests/reader_tests/test_omps_edr.py 98.98% <100.00%> (+0.66%) ⬆️
satpy/readers/fci_l1c_fdhsi.py 96.15% <0.00%> (-0.03%) ⬇️
satpy/tests/writer_tests/test_cf.py 98.53% <0.00%> (ø)
satpy/tests/reader_tests/test_utils.py 100.00% <0.00%> (ø)
satpy/tests/reader_tests/test_ahi_hsd.py 100.00% <0.00%> (ø)
satpy/tests/reader_tests/test_geos_area.py 100.00% <0.00%> (ø)
satpy/tests/reader_tests/test_fci_l1c_fdhsi.py 100.00% <0.00%> (ø)
satpy/tests/reader_tests/test_seviri_l1b_hrit.py 100.00% <0.00%> (ø)
... and 6 more

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 103d380...4820890. Read the comment docs.

@TAlonglong
Copy link
Collaborator Author

I have renamed the datsets to match with the other dataset and marked them with sampo to reflect the data source.

for the test on the hdf5_utils it looks to me this is already covered.

@mraspaud
Copy link
Member

mraspaud commented Apr 1, 2020

I'm good with the changes, looks good to me. One last thing: the reader seems to be missing from the index.rst documentation table, can you update it @TAlonglong ?

@TAlonglong
Copy link
Collaborator Author

OK. Updated.

But I see that the data DIMENSION_LIST stuff we added is not tested. I tried to add test to handle this, but what ever I try the DIMENSION_LIST attribute I add never get to where this is used in the reader.

@mraspaud
Copy link
Member

mraspaud commented Apr 1, 2020

If you want to test the dimension list, it should ideally be a list of references. Actually in the files it looks like this:

variable.attrs['DIMENSION_LIST'] = np.array([np.array([ref1]), np.array([ref2])])

@TAlonglong
Copy link
Collaborator Author

OK, I tried with this: https://github.com/pytroll/satpy/pull/1126/files#diff-c736dc353b90f5ae010c5b8b42896df5R114 but the attribute I set here is not handled. So it never get into the data.attrs. And I dont understand why this attr I set just disapear.

@mraspaud
Copy link
Member

mraspaud commented Apr 1, 2020

I can try to have a look later

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.

LGTM

@djhoese djhoese merged commit c35e8c0 into pytroll:master Apr 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:readers enhancement code enhancements, features, improvements
Projects
None yet
4 participants