-
Notifications
You must be signed in to change notification settings - Fork 19
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
More informative log message when granule covers #77
Conversation
More informative log message when granule covers the area indicated.
DeepCode failed to analyze this pull requestSomething went wrong despite trying multiple times, sorry about that. |
There appear to be no unit tests for |
Add unit test for region collector. Incomplete, but tests the improved logging added in the last commits.
Should we set up CI before proceeding? The new unit tests pass for me, but I'm not very rigorous in testing locally with older Python versions. |
I can have a go adding Github Actions for unit tests tomorrow morning. |
The region gatherer unit test needs pyresample. Install pyresample in GitHub CI.
Unit tests need pytroll-schedule. Add this as a test dependency.
Replace new Python syntax by syntax supported in Python 3.7.
Codecov Report
@@ Coverage Diff @@
## master #77 +/- ##
==========================================
+ Coverage 72.85% 77.71% +4.85%
==========================================
Files 13 14 +1
Lines 2461 2508 +47
==========================================
+ Hits 1793 1949 +156
+ Misses 668 559 -109
Continue to review full report at Codecov.
|
Satisfy codefactor by removing a FIXME and an unneccesary else after return.
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 work! I have some suggestions about the word orderings, take them or leave them 😁
@@ -102,15 +102,14 @@ def collect(self, granule_metadata): | |||
self.granule_times.add(ptime) | |||
self.granules.append(granule_metadata) | |||
self.last_file_added = True | |||
LOG.info("Added %s (%s) granule to area %s", | |||
LOG.info("Added %s (%s) granule to area %s because we expect it", |
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.
LOG.info("Added %s (%s) granule to area %s because we expect it", | |
LOG.info("Added overlapping granule %s (%s) to area %s", |
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.
Chose a slightly different formulation instead. One aim was that I wanted to understand from my logs whether granules were added because they were expected, or because they were newly overlapping.
@@ -127,8 +126,7 @@ def collect(self, granule_metadata): | |||
|
|||
if new_timeout < self.timeout: | |||
self.timeout = new_timeout | |||
LOG.info("Adjusted timeout: %s", | |||
self.timeout.isoformat()) | |||
LOG.info(f"Adjusted timeout for {platform!s}: {self.timeout:%Y-%m-%d %H:%M:%S}") |
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.
LOG.info(f"Adjusted timeout for {platform!s}: {self.timeout:%Y-%m-%d %H:%M:%S}") | |
LOG.info(f"Adjusted timeout for {platform!s} to {self.timeout:%Y-%m-%d %H:%M:%S}") |
@@ -160,7 +161,7 @@ def collect(self, granule_metadata): | |||
|
|||
if not self.planned_granule_times: | |||
self.planned_granule_times.add(start_time) | |||
LOG.info("Added %s (%s) granule to area %s", | |||
LOG.info("Added %s (%s) granule to area %s because it overlaps", |
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.
LOG.info("Added %s (%s) granule to area %s because it overlaps", | |
LOG.info("Added overlapping granule %s (%s) to area %s", |
@@ -21,7 +21,7 @@ jobs: | |||
python-version: ${{ matrix.python-version }} | |||
- name: Install dependencies | |||
run: | | |||
pip install -U pytest pytest-cov trollsift six netifaces watchdog posttroll pyyaml pyinotify | |||
pip install -U pytest pytest-cov trollsift six netifaces watchdog posttroll pyyaml pyinotify pyresample pytroll-schedule |
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.
Good catches!
Improve test coverage in region_collector. Remove code that I'm convinced is never reached.
Co-authored-by: Panu Lahtinen <pnuu+git@iki.fi>
…ctors into improve-logging
Retry DeepCode |
Write a more informative log message when granule does cover the area of interest, and add a unit test for region_collector.
I had a case where a granule appeared to be added to both an Arctic and an Antarctic area:
This doesn't happen systematically, but it should never happen. This PR adds a log message with information about the fraction of coverage that may hopefully help to debug this if/when it happens again (I have not yet been able to construct a reproducible case). In the end it turned out that the log messages were misleading; improved logging such as added in this PR will be helpful.
Also added unit tests for region_collector.