VIIRS L2 Cloud Mask reader #2316

pnuu · 2022-12-08T12:11:33Z

This PR adds a reader for VIIRS L2 Cloud Mask products produced by CSPP.

So far there's only support for reading cloud mask (cloud_mask) and binary cloud mask (cloud_mask_binary).

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

codecov · 2022-12-08T12:26:52Z

Codecov Report

Merging #2316 (899e426) into main (85e629f) will increase coverage by 0.01%.
The diff coverage is 98.42%.

@@            Coverage Diff             @@
##             main    #2316      +/-   ##
==========================================
+ Coverage   94.59%   94.60%   +0.01%     
==========================================
  Files         314      316       +2     
  Lines       47547    47674     +127     
==========================================
+ Hits        44976    45101     +125     
- Misses       2571     2573       +2

Flag	Coverage Δ
behaviourtests	`4.49% <0.78%> (-0.01%)`	⬇️
unittests	`95.24% <100.00%> (+0.01%)`	⬆️

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

Impacted Files	Coverage Δ
satpy/composites/__init__.py	`90.45% <ø> (ø)`
satpy/utils.py	`24.70% <33.33%> (+0.07%)`	⬆️
satpy/readers/viirs_l2.py	`100.00% <100.00%> (ø)`
satpy/tests/reader_tests/test_viirs_l2.py	`100.00% <100.00%> (ø)`
satpy/tests/test_utils.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-12-08T14:55:17Z

Coverage: 95.2% (+0.01%) from 95.187% when pulling 899e426 on pnuu:feature-viirs-cspp-cloud-mask into 85e629f on pytroll:main.

gerritholl · 2023-01-10T09:31:02Z

satpy/readers/viirs_l2.py

+#!/usr/bin/env python
+# -*- coding: utf-8 -*-


Those lines are redundant and do not add anything useful.

Removed in c6c97a4

gerritholl · 2023-01-10T09:31:33Z

satpy/readers/viirs_l2.py

@@ -0,0 +1,155 @@
+#!/usr/bin/env python
+# -*- coding: utf-8 -*-
+# Copyright (c) 2011-2019 Satpy developers


The years seem wrong, should probably be 2022 or 2022-2023.

Updated in c6c97a4

gerritholl · 2023-01-10T09:32:00Z

satpy/readers/viirs_l2.py

+
+from satpy.readers.netcdf_utils import NetCDF4FileHandler
+
+LOG = logging.getLogger(__name__)


I think lowercase, for example, logger, would fit better with style guides.

The logging wasn't even used, so removed it in 0720a1f

gerritholl · 2023-01-10T09:33:02Z

satpy/readers/viirs_l2.py

+        shape = self.get_shape(dataset_id, ds_info)
+        file_units = self._get_dataset_file_units(ds_info, var_path)
+
+        i = getattr(self[var_path], 'attrs', {})


Could this variable have a more descriptive name?

Done in 732ad51

gerritholl · 2023-01-10T09:33:47Z

satpy/readers/viirs_l2.py

+    def _get_dataset_file_units(self, ds_info, var_path):
+        file_units = ds_info.get('file_units')
+        if file_units is None:
+            file_units = self.get(var_path + '/attr/units')


If codecov is right, this could use a test.

Hmm, I think this bit of code isn't even needed, the units are coming from the reader YAML. I'll just remove this if block.

Removed in c32ec3d

And a check that 'units' is in the dataset attributes added in 6e41d23

gerritholl · 2023-01-10T09:36:31Z

satpy/readers/viirs_l2.py

+                yield is_avail, ds_info
+                continue


I don't quite understand what's going on here, but it seems this could also use a test.

Neither do I. This was in the file handler I used as a template.

If you define all the datasets in the yaml file, this function shouldn't be needed at all.

gerritholl · 2023-01-10T09:36:49Z

satpy/tests/reader_tests/test_viirs_l2.py

+#!/usr/bin/env python
+# -*- coding: utf-8 -*-


These lines are redundant and don't add anything.

Removed in 526b454

gerritholl · 2023-01-10T09:38:06Z

satpy/tests/reader_tests/test_viirs_l2.py

+
+    def __init__(self):
+        """Initialize the class."""
+        self._dir = tempfile.mkdtemp()


Would it be possible to use the pytest tmp_path fixture so you don't need to take care of the cleanup yourself?

In which way? Have the fixture passed to every test function and then pass the fixture to JRRCloudMaskFile.__init__()? When would the temp directory removed?

I think it'd be possible to use a class-scoped fixture. In this case the directory would be created before the first test in the class and deleted after the last one. There is also tmp_path_factory which is, I think, always session-scoped. Personally, I would just pass the (function scoped) fixture (or a fixture using the tmp_path fixture) explicitly to every test function needing it.

I don't feel strongly either way though; just a suggestion, no need to do anything as far as I'm concerned if you prefer to handle the mkdtemp yourself the way you've done.

tmp_path has the advantage of taking care of removing the files after it, creating custom path for each tests, etc...

also, I would make a fixture that returns a created file using this class and tmp_path.

After running the tests three times:

$ ls -1 /tmp/pytest-of-lahtinep/ pytest-0 pytest-1 pytest-2 pytest-current $ du -ch /tmp/pytest-of-lahtinep/ 24M /tmp/pytest-of-lahtinep/pytest-1/test_cloud_mask_read_cloud_mas0 24M /tmp/pytest-of-lahtinep/pytest-1/test_cloud_mask_read_latitude0 24M /tmp/pytest-of-lahtinep/pytest-1/test_cloud_mask_read_longitude0 24M /tmp/pytest-of-lahtinep/pytest-1/test_cloud_mas_read_binary_clo0 94M /tmp/pytest-of-lahtinep/pytest-1 24M /tmp/pytest-of-lahtinep/pytest-0/test_cloud_mask_read_cloud_mas0 24M /tmp/pytest-of-lahtinep/pytest-0/test_cloud_mask_read_latitude0 24M /tmp/pytest-of-lahtinep/pytest-0/test_cloud_mask_read_longitude0 24M /tmp/pytest-of-lahtinep/pytest-0/test_cloud_mas_read_binary_clo0 94M /tmp/pytest-of-lahtinep/pytest-0 24M /tmp/pytest-of-lahtinep/pytest-2/test_cloud_mask_read_cloud_mas0 24M /tmp/pytest-of-lahtinep/pytest-2/test_cloud_mask_read_latitude0 24M /tmp/pytest-of-lahtinep/pytest-2/test_cloud_mask_read_longitude0 24M /tmp/pytest-of-lahtinep/pytest-2/test_cloud_mas_read_binary_clo0 94M /tmp/pytest-of-lahtinep/pytest-2 282M /tmp/pytest-of-lahtinep/ 282M total

No cleanup. At all, as far as I can see.

try a couple times more.

from https://docs.pytest.org/en/7.1.x/reference/reference.html#pytest.tmpdir.tmp_path :

By default, a new base temporary directory is created each test session, and old bases are removed after 3 sessions, to aid in debugging. If --basetemp is used then it is cleared each session. See The default base temporary directory.

Forgot to update. Indeed, on the next run the 0 index directories were deleted and new 3 indexed were created. This could potentially leave quite a lot of stuff behind, so maybe there should be a more strict cleaning. But that's another topic outside of this PR.

Oh, and now the tests use tmp_path fixture. I also added a note at the top of the file listing the external fixtures that are used. I'll be creating a PR discussing this in the dev documentation, and possibly also adding the notes to other tests having this issue of seemingly undefined fixtures in them.

gerritholl · 2023-01-10T09:44:33Z

satpy/tests/reader_tests/test_viirs_l2.py

+NUM_COLUMNS = 3200
+NUM_ROWS = 768


Does the test need to have such a large array? Does this make the test slower and use more resources than necessary?

This is the shape of a single array in the actual data, so I'd say "yes, this is the shape it should be". Running all the current tests for the reader takes 0.7 seconds, so it's not a huge burden.

… reader YAML

mraspaud

Looks good, some comments and questions

mraspaud · 2023-01-10T14:30:36Z

satpy/etc/readers/viirs_l2_cloud_mask_nc.yaml

+  short_name: VIIRS CSPP Cloud Mask
+  long_name: VIIRS CSPP Cloud Mask data in NetCDF4 format
+  description: VIIRS CSPP Cloud Mask reader
+  status: alpha


Beta maybe?

Upgraded (not pushed yet).

Done in 23b0a41

satpy/etc/readers/viirs_l2_cloud_mask_nc.yaml

mraspaud · 2023-01-10T14:34:27Z

satpy/tests/reader_tests/test_viirs_l2.py

+
+    def __init__(self):
+        """Initialize the class."""
+        self._dir = tempfile.mkdtemp()


tmp_path has the advantage of taking care of removing the files after it, creating custom path for each tests, etc...

mraspaud · 2023-01-10T14:36:15Z

satpy/tests/reader_tests/test_viirs_l2.py

+
+    def __init__(self):
+        """Initialize the class."""
+        self._dir = tempfile.mkdtemp()


also, I would make a fixture that returns a created file using this class and tmp_path.

mraspaud

Just a few minor comments, but it looks good!

satpy/readers/viirs_l2.py

mraspaud · 2023-01-12T12:55:43Z

satpy/readers/viirs_l2.py

+                yield is_avail, ds_info
+                continue


If you define all the datasets in the yaml file, this function shouldn't be needed at all.

mraspaud · 2023-01-12T12:56:34Z

satpy/utils.py

+        elif isinstance(f, pathlib.Path):
+            local_files.append(f)


codecov is complaining...

That's weird, because without this addition the tests weren't passing locally.

And adding a print inside that branch shows the filename.

Lets see if the complaint is still there after the tests complete for the latest changes.

Added a separate test in 596aa59

pnuu · 2023-01-12T13:09:22Z

If you define all the datasets in the yaml file, this function shouldn't be needed at all.

For some reason can't reply on this in a thread.. Do you mean the whole available_datasets() could be removed?

pnuu · 2023-01-12T13:28:52Z

Ok, seems to work also without the available_datasets() method, so removed it in de48145.

mraspaud · 2023-01-12T13:56:01Z

All good for me, feel free to merge when the tests pass.

pnuu · 2023-01-12T15:07:51Z

And one final fix for ColormapCompositor docstring that caused the RtD build to fail.

Add initial YAML for the VIIRS L2 Cloud Mask reader

ff6849d

pnuu added the component:readers label Dec 8, 2022

pnuu self-assigned this Dec 8, 2022

Add file handler for VIIRS L2 cloud mask

2065920

Add tests for VIIRS L2 cloud mask reader

f76be31

pnuu marked this pull request as ready for review December 14, 2022 10:28

pnuu requested review from djhoese and mraspaud as code owners December 14, 2022 10:28

gerritholl reviewed Jan 10, 2023

View reviewed changes

pnuu added 6 commits January 10, 2023 12:51

Fix header text

c6c97a4

Remove unused logging

0720a1f

Clarify variable naming

732ad51

Remove unnecessary header text

526b454

Remove unnecessary unit handling code, rely on getting the units from…

c32ec3d

… reader YAML

Check that units are in the dataset attributes

6e41d23

mraspaud reviewed Jan 10, 2023

View reviewed changes

Change reader status from alpha to beta

23b0a41

pnuu mentioned this pull request Jan 11, 2023

Fix the pycoast tests further pytroll/pycoast#86

Merged

pnuu added 5 commits January 12, 2023 13:22

Use fixtures for writing the temporary files

07457d8

Fix passing pathlib.Path as filename

97847c6

Add a note listing which external fixtures are used

5830aa2

Merge branch 'main' into feature-viirs-cspp-cloud-mask

2a2ab76

Remove unused import

962580b

pnuu mentioned this pull request Jan 12, 2023

Add an initial section on writing tests #2348

Merged

mraspaud approved these changes Jan 12, 2023

View reviewed changes

Remove unnecessary try/except in getting platform short_name

e8fcaaf

Remove the unnecessary available_datasets() method

de48145

Add a test for pathlib filenames

596aa59

Fix ColormapCompositor docstring

899e426

pnuu merged commit c990b07 into pytroll:main Jan 13, 2023

pnuu deleted the feature-viirs-cspp-cloud-mask branch January 13, 2023 06:14

djhoese mentioned this pull request Jul 18, 2023

Add "viirs_edr" reader #2531

Merged

3 tasks


		from satpy.readers.netcdf_utils import NetCDF4FileHandler

		LOG = logging.getLogger(__name__)

		NUM_COLUMNS = 3200
		NUM_ROWS = 768

VIIRS L2 Cloud Mask reader #2316

VIIRS L2 Cloud Mask reader #2316

Conversation

pnuu commented Dec 8, 2022 • edited Loading

codecov bot commented Dec 8, 2022 • edited Loading

Codecov Report

coveralls commented Dec 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pnuu Jan 10, 2023 • edited Loading

Choose a reason for hiding this comment

mraspaud left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mraspaud left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pnuu commented Jan 12, 2023

pnuu commented Jan 12, 2023

mraspaud commented Jan 12, 2023

pnuu commented Jan 12, 2023

pnuu commented Dec 8, 2022 •

edited

Loading

codecov bot commented Dec 8, 2022 •

edited

Loading

coveralls commented Dec 8, 2022 •

edited

Loading

pnuu Jan 10, 2023 •

edited

Loading