-
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
Implement adaptive FCI chunks padding and create a new GEOVariableSegmentYAMLReader class #1919
Implement adaptive FCI chunks padding and create a new GEOVariableSegmentYAMLReader class #1919
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1919 +/- ##
==========================================
+ Coverage 93.89% 93.94% +0.04%
==========================================
Files 283 283
Lines 42589 42869 +280
==========================================
+ Hits 39991 40275 +284
+ Misses 2598 2594 -4
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
I appreciate it's motivated by FCI, but should the reader perhaps have a more generic name, |
Thank you @gerritholl for your comment, indeed the new YAMLReader can also be formulated generically, that's a good idea to keep the YAMLReaders as instrument-independent as possible. |
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.
Nice job! Few comments inline.
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 creating this PR, it makes sense to create a new class indeed.
I have a few comments inline, but some more general comments here:
- The amount of code for the geo readers is getting big, should we have a separate modules for these?
- Many places have comments explaining exactly what is happening. I understand and appreciate the intent, however I wonder if in some cases these could be made obsolete by having small function with very descriptive names and/or variables with good names?
… of overridden get_empty_segment, cleanup tests, fix typo
@mraspaud @pnuu thank you for your review! I addressed all your points, let me know if you have any more comments. I also added a test to cover an untested overridden method. Regarding the long |
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
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
Thank you for the review and the merge :) |
This PR implements a new YAMLReader to handle FCI chunks. This was needed since a new functionality needed to be implemented: the chunk sizes of the FCI files is configurable by the data producer, hence the gaps to be padded can be of variable sizes. The new YAMLReader computes the chunks to be padded in an adaptive way.
To keep the GEOSegmentYAMLReader code clean and generic, it was decided to create this new class.
New tests have been added. The old tests were passing after minor adaptations for the new code structure (e.g. change from function to class method).
Code check with real data:
Both images look the same, showing that the full-disk padding works and also the geolocation of the padded chunks is correct.
Similarly for SEVIRI HRIT:
PS: On a side, it adds a # type: ignore for the cached_property imports, to avoid issues with the
mypy
git hooks.