adding a reader for ATMS level1b data #2187

BengtRydberg · 2022-08-19T15:44:08Z

This PR adds a reader for the Advanced Technology Microwave Sounder (ATMS) Level 1B data.
The reason for this is that the nowcasting SAF intends to include a pipeline for processing ATMS within the PPS microwave package where satpy is used for reading of level1b data, and it seems that no existing reader for ATMS data is available within satpy.

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

codecov · 2022-08-19T16:28:10Z

Codecov Report

Merging #2187 (093f622) into main (15be5c1) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2187      +/-   ##
==========================================
+ Coverage   94.14%   94.16%   +0.01%     
==========================================
  Files         294      296       +2     
  Lines       45183    45310     +127     
==========================================
+ Hits        42539    42666     +127     
  Misses       2644     2644

Flag	Coverage Δ
behaviourtests	`4.65% <0.00%> (-0.02%)`	⬇️
unittests	`94.81% <100.00%> (+0.01%)`	⬆️

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

Impacted Files	Coverage Δ
satpy/readers/atms_l1b_nc.py	`100.00% <100.00%> (ø)`
satpy/tests/reader_tests/test_atms_l1b_nc.py	`100.00% <100.00%> (ø)`

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

coveralls · 2022-08-19T16:44:14Z

Coverage increased (+0.01%) to 94.762% when pulling 093f622 on BengtRydberg:atms_reader into 15be5c1 on pytroll:main.

djhoese

From a quick scan of this, this looks really well done. Thanks for adding it and making the pull request. I've made a few comments and suggestions. Let me know what you think. What still needs to be done for this to be not a draft?

djhoese · 2022-08-19T17:37:01Z

satpy/readers/atms_l1b_nc.py

+        try:
+            dataset = (
+                self[param] if param not in self.channel_name_to_index
+                else self._get_channel_data(param)
+            )
+        except KeyError:
+            logger.warning(f'Could not find {param} in NetCDF file, no valid Dataset created')  # noqa: E501


Is this try/except needed? Is the error message that is generated by default (without this try/except) too confusing? I'm not sure this adds much to the log. Additionally, I'm not sure returning None is the expected behavior when a dataset can't be loaded. I think KeyError should be handled just fine.

I checked and found out this try/except is not needed. The default error message is good enough, so I removed it.

djhoese · 2022-08-19T17:42:20Z

satpy/readers/atms_l1b_nc.py

+ATMS_CHANNEL_NAME_TO_INDEX = {
+    "1": 0,
+    "2": 1,
+    "3": 2,
+    "4": 3,
+    "5": 4,
+    "6": 5,
+    "7": 6,
+    "8": 7,
+    "9": 8,
+    "10": 9,
+    "11": 10,
+    "12": 11,
+    "13": 12,
+    "14": 13,
+    "15": 14,
+    "16": 15,
+    "17": 16,
+    "18": 17,
+    "19": 18,
+    "20": 19,
+    "21": 20,
+    "22": 21,
+}


I'm not sure I understand why this is needed. Isn't this just int(channel_name) - 1? It might be cleaner and less code overall if in the data loading you did something like:

try: channel_index = int(name) - 1 return self.antenna _temperature[:, :, channel_index] except TypeError: return self[name]

then you don't have to have this dictionary or the overridden __init__.

Thanks, your suggestion is much cleaner. Fixed it.

djhoese · 2022-08-19T17:47:52Z

satpy/etc/readers/atms_l1b_nc.yaml

+    Reader for JPSS Advanced Technology Microwave Sounder Level 1B files in NetCDF4.
+  status: Beta
+  sensors: [atms]
+  reader: !!python/name:satpy.readers.yaml_reader.FileYAMLReader


Suggested change

reader: !!python/name:satpy.readers.yaml_reader.FileYAMLReader

reader: !!python/name:satpy.readers.yaml_reader.FileYAMLReader

supports_fsspec: false

This additional information was added recently and is described here: https://satpy.readthedocs.io/en/latest/dev_guide/custom_reader.html#the-reader-section

thanks for letting me know.

BengtRydberg · 2022-08-19T22:40:27Z

From a quick scan of this, this looks really well done. Thanks for adding it and making the pull request. I've made a few comments and suggestions. Let me know what you think. What still needs to be done for this to be not a draft?

One reason this is a draft PR is that I wanted to include a data_identification_key (frequency_quadruple_sideband) in the yaml file that is currently being developed in #2120

djhoese · 2022-08-21T00:54:56Z

One reason this is a draft PR is that I wanted to include a data_identification_key (frequency_quadruple_sideband) in the yaml file that is currently being developed in #2120

Sounds good. Thanks for letting me know. I'm not sure I'm the right person to review something like that and it looks like Gerrit already got to it. Let me know if you need anything else from me until this PR is further along.

adybbroe · 2022-08-23T15:09:38Z

Oh, this is great @BengtRydberg I had adding ATMS reader on my to-do-list! :-)
I will have a look at this shortly, and I am now trying to close the PR you refer to above!

mraspaud

LGTM. Clean work, just two small comments inline. Regarding the failing test: it's being worked on, so we're waiting for it to be fixed before merging other PRs.

satpy/readers/atms_l1b_nc.py

satpy/tests/reader_tests/test_atms_l1b_nc.py

BengtRydberg · 2022-09-19T10:17:52Z

LGTM. Clean work, just two small comments inline. Regarding the failing test: it's being worked on, so we're waiting for it to be fixed before merging other PRs.

ok.

mraspaud · 2022-09-21T07:18:24Z

@BengtRydberg the tests are fixed in main now, could you merge main into your branch?

BengtRydberg · 2022-09-23T17:48:53Z

@BengtRydberg the tests are fixed in main now, could you merge main into your branch?

I have merged main into the branch, but note that I had to add an rtol in assert_allclose test that is not mine, in order to not fail unit tests in CI. This test seems to test details from another package (skyfield). Maybe I should not have done this?

mraspaud · 2022-11-09T15:18:37Z

@BengtRydberg the tests are fixed in main now, could you merge main into your branch?

I have merged main into the branch, but note that I had to add an rtol in assert_allclose test that is not mine, in order to not fail unit tests in CI. This test seems to test details from another package (skyfield). Maybe I should not have done this?

that's fine.

mraspaud

LGTM

BengtRydberg added 2 commits August 19, 2022 17:32

adding a reader for ATMS level1b data

62fb09a

lint fixes

5af9c94

djhoese requested changes Aug 19, 2022

View reviewed changes

fixing reviewer comments

dcb9d1b

BengtRydberg and others added 2 commits September 16, 2022 18:04

Merge branch 'pytroll:main' into atms_reader

5aa0d99

fixing frequency definition of channels

59a8472

BengtRydberg marked this pull request as ready for review September 16, 2022 16:42

BengtRydberg requested a review from mraspaud as a code owner September 16, 2022 16:42

mraspaud approved these changes Sep 19, 2022

View reviewed changes

satpy/readers/atms_l1b_nc.py Outdated Show resolved Hide resolved

satpy/tests/reader_tests/test_atms_l1b_nc.py Outdated Show resolved Hide resolved

removing unneeded stuff in file header

793f13e

BengtRydberg and others added 2 commits September 23, 2022 16:32

Merge branch 'pytroll:main' into atms_reader

35dfc0a

adding rtol in test

896a720

BengtRydberg requested a review from djhoese September 30, 2022 13:44

BengtRydberg added 2 commits October 11, 2022 11:24

Merge branch 'main' into atms_reader

d6dfbff

adding missing fix

093f622

mraspaud assigned BengtRydberg Nov 9, 2022

mraspaud added enhancement code enhancements, features, improvements component:readers labels Nov 9, 2022

mraspaud approved these changes Nov 9, 2022

View reviewed changes

mraspaud merged commit 5848064 into pytroll:main Nov 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adding a reader for ATMS level1b data #2187

adding a reader for ATMS level1b data #2187

BengtRydberg commented Aug 19, 2022 •

edited

Loading

codecov bot commented Aug 19, 2022 •

edited

Loading

coveralls commented Aug 19, 2022 •

edited

Loading

djhoese left a comment

djhoese Aug 19, 2022

BengtRydberg Aug 19, 2022

djhoese Aug 19, 2022

BengtRydberg Aug 19, 2022

djhoese Aug 19, 2022

BengtRydberg Aug 19, 2022

BengtRydberg commented Aug 19, 2022

djhoese commented Aug 21, 2022

adybbroe commented Aug 23, 2022

mraspaud left a comment

BengtRydberg commented Sep 19, 2022

mraspaud commented Sep 21, 2022

BengtRydberg commented Sep 23, 2022

mraspaud commented Nov 9, 2022

mraspaud left a comment

	reader: !!python/name:satpy.readers.yaml_reader.FileYAMLReader
	reader: !!python/name:satpy.readers.yaml_reader.FileYAMLReader
	supports_fsspec: false

adding a reader for ATMS level1b data #2187

adding a reader for ATMS level1b data #2187

Conversation

BengtRydberg commented Aug 19, 2022 • edited Loading

codecov bot commented Aug 19, 2022 • edited Loading

Codecov Report

coveralls commented Aug 19, 2022 • edited Loading

djhoese left a comment

Choose a reason for hiding this comment

djhoese Aug 19, 2022

Choose a reason for hiding this comment

BengtRydberg Aug 19, 2022

Choose a reason for hiding this comment

djhoese Aug 19, 2022

Choose a reason for hiding this comment

BengtRydberg Aug 19, 2022

Choose a reason for hiding this comment

djhoese Aug 19, 2022

Choose a reason for hiding this comment

BengtRydberg Aug 19, 2022

Choose a reason for hiding this comment

BengtRydberg commented Aug 19, 2022

djhoese commented Aug 21, 2022

adybbroe commented Aug 23, 2022

mraspaud left a comment

Choose a reason for hiding this comment

BengtRydberg commented Sep 19, 2022

mraspaud commented Sep 21, 2022

BengtRydberg commented Sep 23, 2022

mraspaud commented Nov 9, 2022

mraspaud left a comment

Choose a reason for hiding this comment

BengtRydberg commented Aug 19, 2022 •

edited

Loading

codecov bot commented Aug 19, 2022 •

edited

Loading

coveralls commented Aug 19, 2022 •

edited

Loading