-
Notifications
You must be signed in to change notification settings - Fork 303
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
feat: Enable to read for the Q4 coverage and the IQTI files for the fci l1c data #2843
Conversation
ClementLaplace
commented
Jun 28, 2024
•
edited
Loading
edited
- Add support for RSS imagery (Q4 coverage)
- test support for RSS imagery (Q4 coverage)
- Add support for IQTI files
- Test for IQTI files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Clement, thanks for this! Below the first comments.
Another thing: I believe we can get rid of these lines as well
satpy/satpy/readers/fci_l1c_nc.py
Lines 500 to 508 in 9070f29
# TODO remove this check when old versions of IDPF test data (<v4) are deprecated. | |
if coord == "x" and coord_radian.attrs["scale_factor"] > 0: | |
coord_radian.attrs["scale_factor"] *= -1 | |
# TODO remove this check when old versions of IDPF test data (<v5) are deprecated. | |
if type(coord_radian.attrs["scale_factor"]) is np.float32: | |
coord_radian.attrs["scale_factor"] = coord_radian.attrs["scale_factor"].astype("float64") | |
if type(coord_radian.attrs["add_offset"]) is np.float32: | |
coord_radian.attrs["add_offset"] = coord_radian.attrs["add_offset"].astype("float64") |
With that, please kindly also cleanup the according tests and fixtures for the test class `TestFCIL1cNCReaderBadDataFromIDPF´ that is not needed anymore.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2843 +/- ##
==========================================
+ Coverage 95.78% 95.97% +0.18%
==========================================
Files 366 368 +2
Lines 53521 53973 +452
==========================================
+ Hits 51267 51801 +534
+ Misses 2254 2172 -82
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Pull Request Test Coverage Report for Build 10112077539Warning: 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
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates. However, I think some tests and modifications are still missing, please check the inline comments.
…ce is set to 1 for IQTI data
…riod_min, change for the AF data the name from erranuous_count_in_repeat_cycle to count_in_repeat_cycle
…ycle_rc_period_min and test_count_in_repeat_cycle_rc_period_min_AF
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the revisions, looks good! Just a couple of comments and a new small request on the sun_earth_distance calculation.
…o the computing of sun_earth distance
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again! Looks good, some minor comments plus a CodeScene issue to address.
Please also update the documentation parts in the .yaml and the python docstring, specifying that we now support RSS scanning mode as well, and data from both IDPF-I and IQT-I processing facilities.
…r the test_fci_l1c.py files
…lass for the test_fci_l1c.py file
…void errors into the CI/CD
…ing mode as well, and data from both IDPF-I and IQT-I processing facilities.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for all the work Clement! LGTM, just two minor docstring fixes I just committed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple of comments/suggestions, but looks good overall!
Co-authored-by: Martin Raspaud <martin.raspaud@smhi.se>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!