Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix clock drift rounding error #82

Closed
wants to merge 2 commits into from
Closed

Fix clock drift rounding error #82

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
09a4fc5
fix IndexError in POD reader adjust clock drift
Aug 27, 2020
27f3a85
Consider milliseconds of clock offsets
Aug 27, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 9 additions & 7 deletions pygac/pod_reader.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -433,8 +433,9 @@ def _adjust_clock_drift(self):
LOG.info("Adjusting for clock drift of %s to %s",
str(min(offsets)),
str(max(offsets)))
self.times = (self.utcs +
offsets.astype('timedelta64[s]')).astype(datetime.datetime)
self.times = (
self.utcs + (1000*offsets).astype('timedelta64[ms]')
).astype(datetime.datetime)
offsets *= -2

int_offsets = np.floor(offsets).astype(np.int)
Expand All @@ -443,13 +444,14 @@ def _adjust_clock_drift(self):
line_indices = (self.scans["scan_line_number"]
+ int_offsets)

missed = sorted((set(line_indices) |
set(line_indices + 1))
- set(self.scans["scan_line_number"]))
missed = sorted(
(set(range(line_indices.min(), self.scans["scan_line_number"].max() + 1))
| set(line_indices + 1))
- set(self.scans["scan_line_number"])
)

min_idx = min(line_indices)
max_idx = max(max(line_indices),
max(self.scans["scan_line_number"] - min_idx)) + 1
max_idx = max(line_indices.max(), self.scans["scan_line_number"].max()) + 1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why remove min_idx here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because this leads to the index error. The max argument compares line numbers and is therefore in the line number domain. the - min_idx is the transformation to the complete_lons array index domain. In the case of the crashing orbits, we had exactly the situation, where the right side wins the max comparison.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe better variable names could help to avoid this confusion. Something like min_line, max_line, line_range = max_line - min_line. Then we can use index or idx for the indices of complete_lons, complete_lats which are numbers in 0:line_range+1, where the interesting ones are self.scans["scan_line_number"] - min_line.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because this leads to the index error. The max argument compares line numbers and is therefore in the line number domain. the - min_idx is the transformation to the complete_lons array index domain. In the case of the crashing orbits, we had exactly the situation, where the right side wins the max comparison.

From reading again the code, I see, that the right hand side should win the max comparison whenever the clock drift is positive, because the array line_indices results from

pygac/pygac/pod_reader.py

Lines 438 to 444 in 5c59f2b

offsets *= -2
int_offsets = np.floor(offsets).astype(np.int)
# filling out missing geolocations with computation from pyorbital.
line_indices = (self.scans["scan_line_number"]
+ int_offsets)

where offsets is a floating point number in seconds. The multiplication with -2 to convert the time offset into a line number offset is not well documented, but we know that the sign of the offset determines who wins the max comparison. I think the reason why it sill worked in most of the cases is the +2 in the following line:

pygac/pygac/pod_reader.py

Line 453 in 5c59f2b

idx_len = max_idx - min_idx + 2

the line I tested that crashed had an int_offset of -3, which in combination with the unexplained +2 appeared suspicious to me. Since the max index has already the +1, the range of valid indices should not require any +2. My first guess was that the +2 is required for the later interpolation, but slerp is linear and therefore only requires the next neighbours.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now, I assume that it also requires this change to cover all possible indexes

min_idx = min(line_indices.min(), self.scans["scan_line_number"].min())

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And without understanding the transformation, but arguing with symmetry, I would assume that

pygac/pygac/pod_reader.py

Line 440 in 5c59f2b

int_offsets = np.floor(offsets).astype(np.int)

should be changed to

int_offsets = np.where(offsets<0, np.floor(offsets), np.ceil(offsets)).astype(np.int)

to cover the right index range, but here I am not yet 100% sure.

idx_len = max_idx - min_idx + 2

complete_lons = np.full((idx_len, self.lats.shape[1]), np.nan,
Expand Down