search

pytroll / satpy

Python package for earth-observing satellite data processing
http://satpy.readthedocs.org/en/latest/
GNU General Public License v3.0
1.04k stars 287 forks source link

Add VIIRS L2 Reader #2740

Closed wjsharpe closed 3 months ago

wjsharpe commented 5 months ago

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 #2729
    • [ ] Tests added
    • [ ] Fully documented
    • [ ] Add your name to AUTHORS.md if not there already
wjsharpe commented 5 months ago

@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 commented 5 months ago

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 commented 5 months ago

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.

Details

Changes Missing Coverage Covered Lines Changed/Added Lines %
satpy/readers/viirs_l2.py 84 102 82.35%
<!-- Total: 157 175 89.71% -->
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%
<!-- Total: 30 -->
Totals Coverage Status
Change from base Build 7758952336: 0.02%
Covered Lines: 51032
Relevant Lines: 53165
💛 - Coveralls
codecov[bot] commented 5 months ago

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 :warning:
Additional details and impacted files ```diff @@ 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](https://app.codecov.io/gh/pytroll/satpy/pull/2740/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pytroll) | Coverage Δ | | |---|---|---| | [behaviourtests](https://app.codecov.io/gh/pytroll/satpy/pull/2740/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pytroll) | `4.12% <0.00%> (-0.03%)` | :arrow_down: | | [unittests](https://app.codecov.io/gh/pytroll/satpy/pull/2740/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pytroll) | `96.00% <89.71%> (-0.04%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pytroll#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

djhoese commented 4 months ago

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 commented 4 months ago

@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 commented 4 months ago

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 commented 4 months ago

@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 commented 4 months ago

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 commented 4 months ago

@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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

Enhancements are now removed