Add VIIRS L2 Reader

[wjsharpe]

This PR adds the code for the (initial) functionality for the VIIRS Level 2 reader. There is currently no reader available for many SNPP and NOAA20 products we deliver, this should help with that.

Components of PR:

1. VIIRS_L2 reader and corresponding yaml file for four VIIRS_L2 products and other VIIRS_L2 file types. This could be expanded in the future to support all VIIRS_L2 products in NASA Worldview
2. Added four colormaps/palettes to generic.yaml in enhancements (could change these to filename reference if there is a good spot to put the txt files in Satpy project)
3. Added composites to viirs.yaml to correctly mask Cloud Top Height NaN values

• Closes Add Viirs L2 Reader + Enhancments #2729
• Tests added
• Fully documented
• Add your name to AUTHORS.md if not there already

[djhoese]

Nice job! I had a couple requests and questions that I've made inline. My only other question is if there is a reason the enhancements in generic.yaml couldn't be put in viirs.yaml? Sorry if I missed/forgot this in a past discussion.

[wjsharpe]

@djhoese Thanks for looking at my changes! The reason I put the enhancements in generic.yaml instead of viirs.yaml was because those same colormaps are used for the the equivalent products for older instruments (e.g. clear sky confidence with the MODIS instrument). So for now they are only used with VIIRS but if the MODIS_L2 reader was extended they would use those same color/palettes. If you want I could put them in viirs.yaml for now and just keep this in mind for later.

[djhoese]

That's fine then. I don't have a great answer for you regarding where the long colormap files could/should go if not in the YAML. I don't think etc/colormaps/ is unreasonable, but since I don't think we have anything there already it should be discussed. @mraspaud @pnuu thoughts?

[coveralls]

Pull Request Test Coverage Report for Build 7990283687

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 157 of 175 (89.71%) changed or added relevant lines in 2 files are covered.
• 30 unchanged lines in 9 files lost coverage.
• Overall coverage increased (+0.02%) to 95.988%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
satpy/readers/viirs_l2.py	84	102	82.35%

Files with Coverage Reduction	New Missed Lines	%
satpy/tests/test_readers.py	1	99.36%
satpy/tests/reader_tests/test_mersi_l1b.py	1	99.8%
satpy/composites/init.py	1	94.83%
satpy/readers/ahi_hsd.py	1	99.36%
satpy/dataset/metadata.py	1	99.26%
satpy/readers/fci_l2_nc.py	1	99.56%
satpy/readers/mersi_l1b.py	3	97.2%
satpy/readers/init.py	4	98.65%
satpy/readers/clavrx.py	17	93.94%

Totals
Change from base Build 7758952336:	0.02%
Covered Lines:	51032
Relevant Lines:	53165

---

💛 - Coveralls

[codecov]

Codecov Report

Attention: Patch coverage is 89.71429% with 18 lines in your changes are missing coverage. Please review.

Project coverage is 95.90%. Comparing base (f1170f6) to head (fd5655f).
Report is 137 commits behind head on main.

Files	Patch %	Lines
satpy/readers/viirs_l2.py	82.35%	18 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2740      +/-   ##
==========================================
+ Coverage   95.42%   95.90%   +0.48%     
==========================================
  Files         372      377       +5     
  Lines       52944    53342     +398     
==========================================
+ Hits        50524    51160     +636     
+ Misses       2420     2182     -238     

Flag	Coverage Δ
behaviourtests	4.12% <0.00%> (-0.03%)	⬇️
unittests	96.00% <89.71%> (-0.04%)	⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[djhoese]

Given my other comment about potentially moving the filtering/masking to the reader, I think this enhancement specifically should go in enhancements/viirs.yaml given the viirs_ prefix. Additionally, the other enhancements are being applied/set based on the specific name: of the product. Are these names the same for the MODIS data that you were looking at supporting? Or do they differ? Should we maybe have the enhancements controlled by standard_name instead of the specific name? However, I then fear we'd be changing long held default behavior for all instruments/readers that produce these types of products. Essentially making decisions for all satellite instruments/readers based on one NASA Worldview likes. Not that these are bad colormaps (I haven't look at them), but up until now we've been pretty relaxed as far as defaults for enhancements on products. Usually users, even core maintainers of satpy, end up having their own custom etc/ directory with custom enhancements.

[wjsharpe]

That viirs_cloud_top_height is just poorly names by me, it could also be applied to MODIS data. I can change the name of it to reflect that. The names are the same for the MODIS data that I am looking to support in the future. As far as potential downstream behavior, would there be a better way for me to specify how to apply the enhancement? I was hoping to keep it in the project as some of my coworkers will theoretically also be using these colormaps.

[djhoese]

I think there are two possible solutions, either of which is fine and have their own pros and cons. In either case I think having the colormaps as separate files in etc/colormaps/ is fine. In both of the below cases I think you could include the colormap files in Satpy if they are any type of "standard" colormap that people are used to seeing and using, even if they are never used in Satpy itself...although now that I say that maybe that is a little weird to have them and not use them. Anyway...

1. Configure all of these composites and enhancements in Satpy as you have now. This assumes that none of these new definitions conflict with existing definitions and won't cause too much headache down the line.
2. Distribute your own collection of etc/ YAML and colormap files that users include as part of their environment or local working directory to get access to these custom enhancements. There is a plugin system in Satpy that would make this decently easy as a separate python package.

Obviously the best part of the first option is no extra maintenance on your part as far as user installations go. The second option is great because it means you have full control over what your users/colleagues are using, that Satpy's future changes don't modify your configuration, and that we avoid any conflicts with other products/instruments. But there is a maintenance aspect to it. If there are conflicts with existing definitions or other readers/instruments then we're stretching Satpy's historically lazy/relaxed policy on this kind of stuff and proving that we need to add more features for things like "user profiles" to control which set of configurations users are using even when they are all provided by Satpy.

[wjsharpe]

I updated this similar to option 1, but to hopefully mitigate potential headaches I changed my colormaps from using name to standard_name and made the standard_name formatted like filetype+name (e.g. cldprop_cloud_top_height). This naming scheme would still allow reuse for the MODIS data which could come later, but shouldn't conflict with anything else.

[djhoese]

I'm not sure I'm a fan of the filetype appearing in the standard_name. After all we'd like the standard_names to be CF-ish, but I understand why you did it. Let's let others review this PR and we'll get back to this after others have had a chance to think about conflicts.

[djhoese]

Thanks for doing all this work. I had a few more inline comments. Otherwise, could you rename the colormap files to .csv and remove the whitespace?

[djhoese]

Does Satpy find this colormap in etc/colormaps/?

[wjsharpe]

I set the config path to the etc/colormaps/ directory locally before running any of this

[djhoese]

Which config path?

[wjsharpe]

I set SATPY_CONFIG_PATH using satpy.config.set(config_path=['xxx/etc/colormaps/'])

[djhoese]

Could you try not modifying the config path and specify these in the YAML as colormaps/clear_sky_confidenct.csv and so on?

Or does that not work?

[wjsharpe]

That works as well, just updated.

[pnuu]

Couple of comments:

• What if we add other cloud top height colormaps "native" to other such products? Should the naming show which producer these colormaps belong to?
• We have the generic colormaps in Trollimage. Should these be included there? Probably not, as they are data specific. Just a thought.
• All the other enhancement names are lower case.
• Should the colormaps have some headers to explain their source and where they are expected to be used?

[djhoese]

What if we add other cloud top height colormaps "native" to other such products? Should the naming show which producer these colormaps belong to?

Good point. If something has a name, it should be meaningful. And in this shared context they should try to follow some kind of defined standard naming. Otherwise we end up renaming everything in the future like we did with the reader names because everyone followed a different but kind of similar scheme.

We have the generic colormaps in Trollimage. Should these be included there? Probably not, as they are data specific. Just a thought.

Another good point, but I think you're right. You're saying should these (and all?) colormaps be put in trollimage. I feel pretty strongly that the answer is no. Trollimage provides generic functionality, but is for the most part not specific to any task/field.

Should the colormaps have some headers to explain their source and where they are expected to be used?

Maybe a README.txt in etc/colormaps/ that explains each colormap? There may come a day where people are adding numpy binary files for colormaps into this directory in which case they can't add a header so an external description is probably a good place to start and avoids wasted time processing the header every time.

I'm mostly just really uncertain on where to draw the line with this as a user-specific config thing versus builtin functionality. The biggest issue is that some of us (the satpy maintainers) have gotten away with being lazy for the most part with sharing configuration of composites and enhancements and just getting lucky that things mostly don't conflict. In the case of @pnuu and a few others, you've just always had your own custom configs from the start and not depended on Satpy's defaults/builtins. In my Polar2Grid/Geo2Grid projects I depend on Satpy's defaults, but also overwrite a few things the way we want them in our project.

These colormaps and enhancements feel like a user-specific configuration kind of thing...but I/we probably wouldn't feel that way if they didn't involve new colormaps. By that I mean if the enhancement that @wjsharpe wanted was a black and white gamma 0.5 (or 2 or whatever to get square root) for one or more of these products we'd say "yeah, ok, that's not too jarring", but that is still a user-specific configuration that could conflict in the future. So I don't know what the right approach is here.

[wjsharpe]

@djhoese So I think in the interest of trying to get at least the reader + enhancements merged there are a few things I could do. If you think it is too user specific I can remove the /etc/colormaps/ directory and just put them in a shared location for my team and point the config path there. Then after you and the other maintainers decide on some of the specifics discussed above I could be the guinea pig for those changes in a separate merge, if you decide you want them in Satpy at all. For the naming of my enhancements I think how I have it now with using standard_name should avoid any of those conflicts we were talking about last week and would still remain generic enough to be correct for the future Aqua/MODIS version of these products. As for CF compliance of those standard_names, standard_name as far as I know just has the purpose of definitively defining a physical quantity. In this context where there are multiple similar datasets adding an additional modified seems reasonable to accomplish that purpose to me, especially since there is already a set of standard name modifiers that are accepted in the CF standard. Those modifiers are at the end, so I could change to have the product/file type string at the end instead of the beginning if you would prefer (e.g. cloud_top_height_cloudprop instead of cldprop_cloud_top_height).

[djhoese]

As for CF compliance of those standard_names, standard_name as far as I know just has the purpose of definitively defining a physical quantity. In this context where there are multiple similar datasets adding an additional modified seems reasonable to accomplish that purpose to me, especially since there is already a set of standard name modifiers that are accepted in the CF standard. Those modifiers are at the end, so I could change to have the product/file type string at the end instead of the beginning if you would prefer (e.g. cloud_top_height_cloudprop instead of cldprop_cloud_top_height).

Could you point me to the documentation in CF about modifiers on standard names? I didn't think they went as far as user customizations, but were more about defining the specific state of a particular variable. I'd really like to find a way to keep the standard names and if there needs to be non-conflicting enhancements then I think specifying name: and reader: for each product's enhancement is the clearest without complicating/corrupting the standard_name. This may mean duplicate enhancements between VIIRS and MODIS and other sensors but I don't think that's the worst thing in this case.

So I think in the interest of trying to get at least the reader + enhancements merged there are a few things I could do. If you think it is too user specific I can remove the /etc/colormaps/ directory and just put them in a shared location for my team and point the config path there. Then after you and the other maintainers decide on some of the specifics discussed above I could be the guinea pig for those changes in a separate merge, if you decide you want them in Satpy at all.

If you're OK with this, then separating the colormaps out would really clarify what this PR is. Especially if you go as far as separating out the enhancements/ YAML changes. Note that in addition to saving them in a shared location you could make a plugin (https://satpy.readthedocs.io/en/stable/dev_guide/plugins.html) package that could be installed by your colleagues.

Unless @mraspaud (who is on a business trip right now) has a decently positive reaction to including these colormaps in Satpy and definitely wants them in this PR, I think my preference is leaning towards separating them out for now.

[djhoese]

@wjsharpe Just for the record, are these colormaps used by any other organizations/tools? For example AWIPS (VIIRS or otherwise) or maybe a non-US based tool?

[mraspaud]

Thanks for asking for my opinion here. I would actually prefer to keep the colourmaps outside if possible. If you feel the colourmaps would still be of use to other users, I can recommend making and distributing a satpy plugin for it.
Other than that, this PR looks quite good to me.

[wjsharpe]

@djhoese I just made the change separating out the colormaps as well as identifying the enhancements by name and reader instead of standard_name. One thing I would prefer is keeping the enhancement yaml changes. There won't be any conflicts with the convention of specifying the name and reader discussed above, and removing them would necessitate the creation of a plugin like you mentioned. This seems like additional development work for myself and anyone using the reader. In my opinion not having the enhancements would somewhat render the remaining changes in this PR not particularly useful without the plugin, at least for what is currently planned. If you feel strongly about having those changes removed I can do so, as my main priority is resolving this PR as quickly as possible to move onto downstream tasks outside of the Satpy project.

As far as other usages of the colormaps, I'm not positive of their usages outside of worldview. I haven't seen them anywhere else, but I am also not familiar with AWIPS.

[djhoese]

I'm going on parental leave in a little bit, but while waiting in the hospital I had time to read this. If the enhancements that use the colormaps stay in Satpy then the colormaps do too. Right? Or am I missing something with the way you've done the change. If you're distributing the colormaps in an etc directory to colleagues then you can include the enhancement YAML alongside. With or without a full plugin package that should work if the config path includes the location of this extra etc directory.

[mraspaud]

Well, without the csv files, someone wanting to visualise the product in the current state will just get a crash. If you take out the enhancements on the other hand, we would at least get a gray-scale image to look at...
As @djhoese says, if you provide the enhancement config alongside the csv files, you're good.

[wjsharpe]

Enhancements are now removed

[djhoese]

This looks good to me. Thanks for all the back and forth even if it ultimately meant removing the enhancements and colormaps. I've updated the title and original description to not give the wrong impression to people stumbling on the PR.

It is a little late for me here and I don't fully trust myself to merge this tonight. I'll merge this tomorrow after one more look, but other maintainers can feel free to merge this before if it looks good to them.