-
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
Anticipate filehandler sorting in GEOSegmentYAMLReader
to have sorted handlers also with pad_data=False
#2692
Anticipate filehandler sorting in GEOSegmentYAMLReader
to have sorted handlers also with pad_data=False
#2692
Conversation
…s also when pad_data is False
…ed filehandlers and change test accordingly
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
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2692 +/- ##
==========================================
+ Coverage 95.32% 95.35% +0.03%
==========================================
Files 371 371
Lines 52441 52485 +44
==========================================
+ Hits 49988 50049 +61
+ Misses 2453 2436 -17
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 7221665410Warning: This coverage report may be inaccurate.We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
💛 - 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
This PR fixes the issue reported in #2588 by making the segment sorting happen early during
create_filehandlers
, so that it acts for bothpad_data=True
andFalse
.The root cause was that the filehandler sorting by segment was done only in the default
pad_data=True
branch inside the padding mechanism ofGEOSegmentYAMLReader
. When calling e.g.scn.load(['ir_105'], pad_data=False, upper_right_corner='NE')
, in the previous implementation the filehandlers were being sorted (in the baseFileYAMLReader.create_filehandlers
) only bystart_time
(which is always the same after #2420), and then alphabetically by filenames. This is problematic for FCI where sorting alphabetically means sorting byprocessing_time
, and there are cases where theprocessing_time
order does not correspond to the chunk order (due to parallel processing), causing the issue.Note that even without #2420, some FCI segments have the same filename
start_time
, hence the bug would have happened anyway.