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

Change sensor attribute to lowercase in FCI L2 NetCDF reader #3048

Merged

Conversation

strandgren
Copy link
Collaborator

Following a discussion on slack (https://pytroll.slack.com/archives/CK9TPLCG0/p1737127470400239 and https://pytroll.slack.com/archives/C0LNH7LMB/p1737561433643359), this PR changes the sensor name dataset attribute in the FCI L2 NetCDF reader (fci_l2_nc) from uppercase to lowercase. This is needed in order to identify instrument specific composites and enhancements. I also modified the tests to mimic the real files where there sensor and platform information are stored in uppercase in the file metadata, such that we can also check that the sensor attribute is still returned with lowercase.

As discussed on slack, it could be worthwhile updating the documentation as well. However, I'm a bit confused and therefore not sure what and how to update. In the documentation for adding custom readers (https://satpy.readthedocs.io/en/v0.53.0/dev_guide/custom_reader.html) it says that the sensor(s) listed in the yaml file, "must be all lowercase letters for full support throughout in Satpy". However, this was already the case before the PR:

reader:
  name: fci_l2_nc
  short_name: FCI L2 NetCDF4
  long_name: MTG FCI L2 data in netCDF4 format
  description: Reader for EUMETSAT MTG FCI L2 files in NetCDF4 format.
  status: Alpha
  supports_fsspec: false
  sensors: [fci]
  reader: !!python/name:satpy.readers.yaml_reader.GEOFlippableFileYAMLReader

Hence, it seems like this is not enough and that the sensor attribute of the dataset also has to be in lowercase, which is not mentioned here: https://satpy.readthedocs.io/en/stable/reading.html#dataset-metadata (sensor is not even listed as mandatory metadata/attribute). Hence, my question is whether there is any link between the sensor(s) listed in the yaml file and the sensor attribute added as metadata to a dataset? To me it seems like it's not the case, and if so we might want to add a note under the dataset metadata (https://satpy.readthedocs.io/en/stable/reading.html#dataset-metadata), similar to what we have for the reader yaml-file, e.g. something like this:

"sensor - name of the sensor. For full support throughout in Satpy this must be all lowercase letters."

  • Tests adapted
  • Fully documented

Copy link

codecov bot commented Feb 3, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.11%. Comparing base (01237e2) to head (91e758a).
Report is 65 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3048   +/-   ##
=======================================
  Coverage   96.11%   96.11%           
=======================================
  Files         383      383           
  Lines       55673    55673           
=======================================
  Hits        53511    53511           
  Misses       2162     2162           
Flag Coverage Δ
behaviourtests 3.89% <0.00%> (ø)
unittests 96.21% <100.00%> (ø)

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.

@coveralls
Copy link

coveralls commented Feb 3, 2025

Pull Request Test Coverage Report for Build 13131140073

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

  • 13 of 13 (100.0%) changed or added relevant lines in 2 files are covered.
  • 8 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.01%) to 96.222%

Files with Coverage Reduction New Missed Lines %
satpy/tests/utils.py 2 93.16%
satpy/tests/reader_tests/gms/test_gms5_vissr_l1b.py 3 98.67%
satpy/tests/reader_tests/gms/test_gms5_vissr_navigation.py 3 97.18%
Totals Coverage Status
Change from base Build 13089264056: -0.01%
Covered Lines: 53758
Relevant Lines: 55869

💛 - Coveralls

@djhoese
Copy link
Member

djhoese commented Feb 3, 2025

My guess is that the documentation about the YAML sensor was written when it was assumed that file handlers and the base reader was using that YAML sensor. As more readers were developed I think this quickly became not true as most could get the sensor information from the file content.

I think your suggestion of adding the sensor information to the metadata section would be good. I think we should leave the sensor information about the YAML as that is still sometimes used I think with find_files_and_readers. Your suggestion was:

sensor - name of the sensor. For full support throughout in Satpy this must be all lowercase letters.

Small grammar fix: remove the "in" before Satpy.

I wonder if we should add something about needing it to be consistent across readers? Also there is support for multiple sensors as a set object. Maybe:

sensor - name of the sensor that recorded the data. For full support through Satpy this should be all lowercase. If the data is the result of observations from multiple sensors a set object can be used to specify more than one sensor name.

@strandgren
Copy link
Collaborator Author

I wonder if we should add something about needing it to be consistent across readers? Also there is support for multiple sensors as a set object. Maybe:

sensor - name of the sensor that recorded the data. For full support through Satpy this should be all lowercase. If the data is the result of observations from multiple sensors a set object can be used to specify more than one sensor name.

I like your suggestion, I've added it to the docs now. The docs build fails for the PR though, however it also failed before I edited the docs file, so I think it's something unrelated?

@djhoese djhoese merged commit ec8f323 into pytroll:main Feb 4, 2025
16 of 18 checks passed
@djhoese
Copy link
Member

djhoese commented Feb 4, 2025

Thanks! The RTD failure was the one that should be fixed in main so I've merged this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants