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

Add reader for MWS onboard EPS-SG-A #2120

Merged
merged 27 commits into from
Sep 9, 2022

Conversation

adybbroe
Copy link
Contributor

@adybbroe adybbroe commented Jun 2, 2022

This is a first draft reader for the EPS-SG MWS level-1b netCDF file format.

  • Closes #xxxx
  • Tests added
  • Fully documented
  • Add your name to AUTHORS.md if not there already

This is an early draft so far, and several things needs to be addressed:

  • Rearrange the FrquencyRange and related classes defined in the AAPP-MHS reader so they can be reused in several readers
  • Add unittests for the reader
  • Decide how the channel comparisons should behave!
  • Add handling of auxiliary data sets, see FCI and VII readers

Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
@adybbroe
Copy link
Contributor Author

adybbroe commented Jun 2, 2022

Here a sketch of some parts of the MWS temperature sounding channels, to illustrate the quadruple side bands (as for instance also seen on AMSU-A):

quadrouble_side_band_mws

@codecov
Copy link

codecov bot commented Jun 2, 2022

Codecov Report

Merging #2120 (40e8461) into main (acd0745) will increase coverage by 0.10%.
The diff coverage is 98.93%.

@@            Coverage Diff             @@
##             main    #2120      +/-   ##
==========================================
+ Coverage   94.03%   94.14%   +0.10%     
==========================================
  Files         289      293       +4     
  Lines       44564    45091     +527     
==========================================
+ Hits        41904    42449     +545     
+ Misses       2660     2642      -18     
Flag Coverage Δ
behaviourtests 4.68% <0.00%> (-0.07%) ⬇️
unittests 94.79% <98.93%> (+0.09%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
satpy/readers/yaml_reader.py 97.50% <ø> (+0.22%) ⬆️
satpy/readers/pmw_channels_definitions.py 97.60% <97.60%> (ø)
satpy/readers/mws_l1b.py 98.51% <98.51%> (ø)
satpy/readers/aapp_mhs_amsub_l1c.py 95.89% <100.00%> (+2.76%) ⬆️
satpy/tests/reader_tests/test_mws_l1b_nc.py 100.00% <100.00%> (ø)
satpy/tests/test_dataset.py 100.00% <100.00%> (ø)
satpy/tests/test_yaml_reader.py 99.60% <100.00%> (ø)
satpy/tests/reader_tests/test_hrit_base.py 96.55% <0.00%> (-1.67%) ⬇️
satpy/readers/utils.py 91.95% <0.00%> (-0.39%) ⬇️
satpy/utils.py 24.69% <0.00%> (-0.31%) ⬇️
... and 23 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@coveralls
Copy link

coveralls commented Jun 2, 2022

Coverage Status

Coverage increased (+0.1%) to 94.741% when pulling 40e8461 on adybbroe:add-epssg-mws-level1b-reader into acd0745 on pytroll:main.

@adybbroe
Copy link
Contributor Author

adybbroe commented Jun 8, 2022

I have a hard time deciding how the channel/band inter-comparison should behave here! :-|

Adam.Dybbroe added 2 commits June 14, 2022 11:13
Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
@adybbroe adybbroe self-assigned this Jun 14, 2022
@adybbroe adybbroe added enhancement code enhancements, features, improvements component:readers labels Jun 14, 2022
Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
@adybbroe
Copy link
Contributor Author

adybbroe commented Jun 15, 2022

Here, a quick example usage:

In [1]: from satpy import Scene
In [2]: AREAID = 'nsper_swe'
In [3]: FILENAMES = ['/path-to-my-epssg-mws-testdata/W_XX-EUMETSAT-Darmstadt,SAT,SGA1-MWS-1B-RAD_C_EUMT_20210609095009_G_D_20070912084321_20070912102225_T_N____.nc']
In [4]: scn = Scene(filenames=FILENAMES, reader='mws_l1b_nc')
In [5]: from satpy.dataset import DataQuery
In [6]: frq_24 = DataQuery(frequency_range=23.8)
In [7]: scn.load([frq_24])
In [8]: local = scn.resample(AREAID)
In [9]: local.show(frq_24)

It is also possible to load the data simply by specifying the channel number, like: scn.load(['1'])

mws_testdata_23_8_bw

@adybbroe adybbroe marked this pull request as ready for review June 15, 2022 11:15
@adybbroe adybbroe requested review from gerritholl and pnuu June 15, 2022 11:18
Adam.Dybbroe added 2 commits June 23, 2022 13:10
Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
Copy link
Member

@gerritholl gerritholl left a comment

Choose a reason for hiding this comment

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

Lots of new content in a part of Satpy I'm not too familiar with, so do with my questions and remarks what you wish. There seems to be some code duplication, though.

return False

def distance(self, value):
"""Get the distance from value."""
Copy link
Member

Choose a reason for hiding this comment

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

Here too could be some info on how this distance is defined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added more explanation.

Comment on lines 360 to 374
def __lt__(self, other):
"""Compare to another frequency."""
if other is None:
return False
return super().__lt__(other)

def __gt__(self, other):
"""Compare to another frequency."""
if other is None:
return True
return super().__gt__(other)

def __hash__(self):
"""Hash this tuple."""
return tuple.__hash__(self)
Copy link
Member

Choose a reason for hiding this comment

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

Seems to repeat lines 94–108. Could this be refactored out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is correct. I also discussed with @mraspaud that one could probably define the quadruple side band from the double side band, and then use inheritance to get rid of this redundancy. However, I am not sure we will ever find use with all the arithmetics on these bands. For instance I have chosen not to try to be too smart when calculating distances etc between them. Thus I would rather leave such refactoring till we really see a need for this.
I also don't have much time/resources for now to elaborate much further on this.

Copy link
Member

Choose a reason for hiding this comment

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

I think the refactoring can still be done using a mixin class though, even if the link between double- and quadrupleband isn't implemented.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! I did this just now.

Adam.Dybbroe added 4 commits August 23, 2022 15:16
# Conflicts:
#	doc/source/index.rst
Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
Adam.Dybbroe added 3 commits August 24, 2022 14:37
Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
Adam.Dybbroe added 10 commits August 26, 2022 16:13
Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
…alue

Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
…ency types

Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
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.

Still some comments, sorry...

Comment on lines +47 to +52
MWS_CHANNEL_NAMES_TO_NUMBER = {'1': 1, '2': 2, '3': 3, '4': 4,
'5': 5, '6': 6, '7': 7, '8': 8,
'9': 9, '10': 10, '11': 11, '12': 12,
'13': 13, '14': 14, '15': 15, '16': 16,
'17': 17, '18': 18, '19': 19, '20': 20,
'21': 21, '22': 22, '23': 23, '24': 24}
Copy link
Member

Choose a reason for hiding this comment

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

wouldn't be simpler to do int(channel_name) than using a dict like this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was not sure this would actually be the final channel/band naming in the file

Comment on lines +255 to +257
if 'scale_factor' in variable.attrs and 'add_offset' in variable.attrs:
missing_value = variable.attrs['missing_value']
variable.data = da.where(variable.data == missing_value, np.nan,
Copy link
Member

Choose a reason for hiding this comment

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

Is this tested?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably not. I think this code should never be used as all datasets will have a scale and ofsset in the netCDF file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Forget what I said above! The code is now tested, thx!

Comment on lines 175 to 180
left_side_dist = abs(value.central - value.side - value.sideside - left_left)
right_side_dist = abs(value.central + value.side + value.sideside - right_right)
except AttributeError:
if isinstance(value, (tuple, list)):
msg = 'Distance to a quadruple side band frequency not supported for this type'
raise NotImplementedError(msg)
Copy link
Member

Choose a reason for hiding this comment

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

Is this tested?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You got me! Right, it wasn't. It is now. But the NotImplementedError would never happen as it is implemented right now. So, I removed the check for tuple/list.

Adam.Dybbroe and others added 4 commits September 2, 2022 11:07
Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
Simplify - reuse already created list

Co-authored-by: Martin Raspaud <martin.raspaud@smhi.se>
Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
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. Regarding the title, is this still a first draft?

@adybbroe adybbroe changed the title Add first draft reader for MWS onboard EPS-SG-A Add reader for MWS onboard EPS-SG-A Sep 5, 2022
@adybbroe
Copy link
Contributor Author

adybbroe commented Sep 5, 2022

LGTM. Regarding the title, is this still a first draft?

I changed the title. It is adding a reader. Then as this has been developed on test data and we cannot start testing and use it in real applications I suppose there might be things we would like to change and improve. But we don't know yet.

@mraspaud mraspaud merged commit a4417c6 into pytroll:main Sep 9, 2022
@adybbroe adybbroe mentioned this pull request Oct 5, 2022
4 tasks
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
Development

Successfully merging this pull request may close these issues.

4 participants