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

Fix the handling of AMVs unit to units by applying suggestion in #2898 #3031

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

YouvaEUMex
Copy link
Contributor

As describved in #2898, the AMV datasets returned by the fci_l2_nc reader contain the attribute unit, rather than units, which is the CF naming convention. This is an inherited issues since the FCI L2 NetCDF files use the unit attribute, which is then read and used by the reader.

This PR apply the work around developed for other FCI L2 product file handler to the AMVs.

To that extend it slightly refactor FciL2CommonFunctions._set_attributes to handle the AMVs product (not pixels nor segmented) and change slightly the interface to the segmented( that is also changed in the dedicated get_dataset)

The changes are already described in #2898

Copy link

codecov bot commented Jan 14, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.11%. Comparing base (0b9760c) to head (d42c00d).
Report is 5 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3031      +/-   ##
==========================================
- Coverage   96.12%   96.11%   -0.02%     
==========================================
  Files         383      383              
  Lines       55577    55577              
==========================================
- Hits        53424    53416       -8     
- Misses       2153     2161       +8     
Flag Coverage Δ
behaviourtests 3.91% <0.00%> (ø)
unittests 96.20% <100.00%> (-0.02%) ⬇️

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 Jan 14, 2025

Pull Request Test Coverage Report for Build 12793012035

Details

  • 15 of 15 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 96.217%

Totals Coverage Status
Change from base Build 12766191418: 0.0%
Covered Lines: 53664
Relevant Lines: 55774

💛 - Coveralls

Copy link
Collaborator

@strandgren strandgren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this @YouvaEUMex. I've left one comment in line. I would also suggest removing the _get_global_attributes method in FciL2NCAMVFileHandler and instead add a check in the corresponding method in FciL2CommonFunctions to add the channel information in case of reading AMV data.

I

satpy/readers/fci_l2_nc.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@strandgren strandgren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest three minor changes on the updated code.

satpy/readers/fci_l2_nc.py Outdated Show resolved Hide resolved
satpy/readers/fci_l2_nc.py Outdated Show resolved Hide resolved
satpy/readers/fci_l2_nc.py Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

units attribute is not CF conform for the for AMV datasets in the fci_l2_nc reader
3 participants