-
Notifications
You must be signed in to change notification settings - Fork 295
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
Add support for MERSI-1 on FY-3A/B/C #2795
Conversation
pre-commit.ci autofix |
@djhoese One of the CI test keeps failing on
|
Yes. This is a known problem with a scipy update causing a test failure. It is being looked at. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2795 +/- ##
==========================================
- Coverage 95.95% 95.94% -0.01%
==========================================
Files 379 379
Lines 53888 53764 -124
==========================================
- Hits 51708 51585 -123
+ Misses 2180 2179 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
I know this is still a draft, but I'm curious if there are actual differences between the datasets for the 3 satellite instruments. If they're the same channels then maybe the 3 readers could be combined into a single "mersi_l1b" reader?
Well it's kinda annoying here that there are no genuine differences inside the datasets but they are still organized in different forms. So I think merging them into one could be a little bit messy for the users... |
Yeah MERSI and AGRI were the two instruments I think of with designs like that (new version of the instrument, but completely different numbering/ordering of channels). |
pre-commit.ci autofix |
pre-commit.ci autofix |
@djhoese Some of the CI failed again. But at least the tests passed. |
I restarted the CI. Very odd error from the test coverage reporting tool so let's see if it fixes itself... |
Failed again.... |
Looks like a major release of the coveralls python package causing issues: https://pypi.org/project/coveralls/ I have appointments half of today, but I'll try debugging later. |
Pull Request Test Coverage Report for Build 9369822041Warning: 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.
LGTM, thanks a lot for refactoring the tests! Just one comment inline, but after that I can merge
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
Datasets for test:
https://drive.google.com/file/d/1cCoC14AioC2l34jFfxa19F88k5NXuqpi/view?usp=sharing
FY-3A:
FY_3B:
FY_3C: