Repository: pytroll/pygac

Issue:
gac_pod reader is parsing padding bytes as scanlines
I am seeing a lot of warnings reading GAC POD files due to incorrect number of scanlines. As far as I can tell this because the POD reader does not distinguish physical and logical records. From the POD user manual, GAC files store the data in "physical" records (6440 bytes) each containing two "logical" records (3220 bytes)

* The header is written to the first physical record. As gac_pod.py is using `self.offset = 3220` it will attempt to parse the header padding as an extra scanline.

* Each scanline occupies one logical record (i.e. two scans per physical record). If the dataset contains an odd number of scanlines the final physical record will contain padding for the final logical record.

Pygac is currently reading to the end of the file, so all POD files will produce at least 1 or 2 extra scanlines - though these should be removed by `correct_scan_line_numbers`

This would be easy to fix in the reader, but are there any cases where pygac is reading useful data beyond the expected end of the file?

Hints from issue comments:
@sfinkens: Interesting find! So you're saying the first scanline is always invalid because it contains the spare 3220 header bytes? 

I agree that it is probably removed by `correct_scan_line_numbers` or invalid coordinates or other checks. Nevertheless it would be nice to get rid of these warnings.

> are there any cases where pygac is reading useful data beyond the expected end of the file?

None that I'm aware of@oembury: Yes - the first "scanline" is just the spare header bytes (and similarly for the last scanline read if there are an odd number of actual scanlines)

Solution patch:
diff --git a/doc/source/introduction.rst b/doc/source/introduction.rst
index e8980b9..e3f90be 100644
--- a/doc/source/introduction.rst
+++ b/doc/source/introduction.rst
@@ -22,9 +22,9 @@ comprehensive `introduction`_.
 .. _NOAA CLASS:
     https://www.class.noaa.gov/
 .. _POD:
-    https://www.ncei.noaa.gov/pub/data/satellite/publications/podguides/TIROS-N%20thru%20N-14/
+    https://github.com/user-attachments/files/17407676/POD-Guide.pdf
 .. _KLM:
-    https://www.ncei.noaa.gov/pub/data/satellite/publications/podguides/N-15%20thru%20N-19/
+    https://github.com/user-attachments/files/17361330/NOAA_KLM_Users_Guide.pdf
 .. _introduction:
     https://www.class.noaa.gov/release/data_available/avhrr/index.htm
 
diff --git a/pygac/gac_pod.py b/pygac/gac_pod.py
index bc81416..317735b 100644
--- a/pygac/gac_pod.py
+++ b/pygac/gac_pod.py
@@ -64,7 +64,13 @@ class GACPODReader(GACReader, PODReader):
         """Init the GAC POD reader."""
         GACReader.__init__(self, *args, **kwargs)
         self.scanline_type = scanline
-        self.offset = 3220
+        # GAC POD files were originally written to tapes using 6440 byte physical records
+        # with two 3220 logical records per physical record. As the header is in the first
+        # physical record the scanline data will start at offset 6440 in the filestream.
+        # This means the second logical record (at offset 3220) is not used and contains
+        # junk / padding data. The data often appears to be a valid scanline, but it is a
+        # duplicate of a data appearing later in the file and should not be used.
+        self.offset = 6440
         self.scan_points = np.arange(3.5, 2048, 5)
 
 
diff --git a/pygac/klm_reader.py b/pygac/klm_reader.py
index 799056f..ef2db94 100644
--- a/pygac/klm_reader.py
+++ b/pygac/klm_reader.py
@@ -685,7 +685,8 @@ class KLMReader(Reader):
             buffer = fd_.read()
             count = self.head["count_of_data_records"]
             self._read_scanlines(buffer, count)
-        self.correct_scan_line_numbers()
+        if self.correct_scanlines:
+            self.correct_scan_line_numbers()
         self.spacecraft_id = self.head["noaa_spacecraft_identification_code"]
         self.spacecraft_name = self.spacecraft_names[self.spacecraft_id]
 
diff --git a/pygac/pod_reader.py b/pygac/pod_reader.py
index 0990e09..87e5727 100644
--- a/pygac/pod_reader.py
+++ b/pygac/pod_reader.py
@@ -38,6 +38,7 @@ Format specification can be found in chapters 2 & 3 of the `POD user guide`_.
 
 import datetime
 import logging
+import warnings
 
 try:
     from enum import IntFlag
@@ -246,7 +247,7 @@ class PODReader(Reader):
     def correct_scan_line_numbers(self):
         """Correct the scan line numbers."""
         # Perform common corrections first.
-        super().correct_scan_line_numbers()
+        results = super().correct_scan_line_numbers()
 
         # cleaning up the data
         min_scanline_number = np.amin(
@@ -259,6 +260,7 @@ class PODReader(Reader):
                 self.scans = self.scans[1:]
 
         self.scans = self.scans[self.scans["scan_line_number"] != 0]
+        return results
 
     def read(self, filename, fileobj=None):
         """Read the data.
@@ -287,12 +289,14 @@ class PODReader(Reader):
             # read scan lines until end of file
             fd_.seek(self.offset + tbm_offset, 0)
             buffer = fd_.read()
+            buffer = self._truncate_padding_record(buffer)
             count = self.head["number_of_scans"]
             self._read_scanlines(buffer, count)
         year, jday, _ = self.decode_timestamps(self.head["start_time"])
         start_date = (datetime.date(year, 1, 1) +
                       datetime.timedelta(days=int(jday) - 1))
-        self.correct_scan_line_numbers()
+        if self.correct_scanlines:
+            self.correct_scan_line_numbers()
         self.spacecraft_id = self.head["noaa_spacecraft_identification_code"]
         if self.spacecraft_id == 1 and start_date < datetime.date(1982, 1, 1):
             self.spacecraft_id = 25
@@ -301,6 +305,35 @@ class PODReader(Reader):
             "Reading %s data", self.spacecrafts_orbital[self.spacecraft_id])
         return self.head, self.scans
 
+    def _truncate_padding_record(self, buffer):
+        """Remove the padding record (if present) from the data stream.
+
+        In POD files each scanline was stored as a "logical" record, and these
+        were written to the tapes in "physical" records. In the case of GAC
+        data there were two logical records (scanlines) per physical record,
+        so an odd number of scanlines would result in the final physical record
+        containing only one scanline and one "logical" record of padding.
+        """
+        scanlines_per_record = self.offset // self.scanline_type.itemsize
+        # Do nothing if the logical and physical records are the same size (LAC).
+        if scanlines_per_record == 1:
+            return buffer
+        # File should contain an integer number of physical records
+        n_logical = len(buffer) // self.scanline_type.itemsize
+        if n_logical % scanlines_per_record != 0:
+            LOG.warning("Unexpected record length for POD file (incomplete physical record?)")
+            warnings.warn("Unexpected record length for POD file (incomplete physical record?)",
+                          category=RuntimeWarning, stacklevel=2)
+        # How many physical / logical records do we expect based on the file header?
+        expected_scanlines = self.head["number_of_scans"]
+        expected_physical = (expected_scanlines + scanlines_per_record - 1) // scanlines_per_record
+        expected_logical = expected_physical * scanlines_per_record
+        # Only trim the padding if the file is the expected size, so any unexpected
+        # cases will still be dealt with in _read_scanlines
+        if n_logical == expected_logical and n_logical > expected_scanlines:
+            buffer = buffer[:expected_scanlines * self.scanline_type.itemsize]
+        return buffer
+
     @classmethod
     def read_header(cls, filename, fileobj=None, header_date="auto"):
         """Read the file header.
diff --git a/pygac/reader.py b/pygac/reader.py
index 1883727..02b75a8 100644
--- a/pygac/reader.py
+++ b/pygac/reader.py
@@ -103,7 +103,8 @@ class Reader(ABC):
     def __init__(self, interpolate_coords=True, adjust_clock_drift=True,
                  tle_dir=None, tle_name=None, tle_thresh=7, creation_site=None,
                  custom_calibration=None, calibration_file=None, header_date="auto",
-                 calibration_method="noaa", calibration_parameters=None):
+                 calibration_method="noaa", calibration_parameters=None,
+                 correct_scanlines=True):
         """Init the reader.
 
         Args:
@@ -120,6 +121,7 @@ class Reader(ABC):
                                 calibration coefficients
             calibration_file: path to json file containing default calibrations
             header_date: the date to use for pod header choice. Defaults to "auto".
+            correct_scanlines: Remove corrrupt scanline numbers. Defaults to True
 
         """
         self.meta_data = {}
@@ -130,6 +132,7 @@ class Reader(ABC):
         self.tle_thresh = tle_thresh
         self.creation_site = (creation_site or "NSS").encode("utf-8")
         self.header_date = header_date
+        self.correct_scanlines = correct_scanlines
         self.head = None
         self.scans = None
         self.spacecraft_name = None


Test patch:
diff --git a/pygac/tests/test_pod.py b/pygac/tests/test_pod.py
index c006ab4..35efde5 100644
--- a/pygac/tests/test_pod.py
+++ b/pygac/tests/test_pod.py
@@ -291,3 +291,69 @@ class TestPOD(unittest.TestCase):
         get_offsets.return_value = np.zeros(10), np.zeros(10)
         get_tle_lines.side_effect = NoTLEData("No TLE data available")
         reader._adjust_clock_drift()  # should pass without errors
+
+
+class TestPOD_truncate(unittest.TestCase):
+    """Test the POD GAC reader."""
+
+    longMessage = True
+
+    def setUp(self):
+        """Set up the test."""
+        self.reader = GACPODReader()
+
+    def test__two_logical_record(self):
+        """Test that truncate_padding_record is correctly dropping end padding"""
+        reader = self.reader
+        # GAC POD should have two logical records per physical
+        self.assertEqual(reader.offset // reader.scanline_type.itemsize, 2)
+
+    def test__truncate_padding_record_even_correct(self):
+        """Test that truncate_padding_record is correctly dropping end padding"""
+        # Even number of scans, correct file length
+        reader = self.reader
+        reader.head = {'number_of_scans': 10}
+        buffer = reader._truncate_padding_record(bytes(10*3220))
+        self.assertEqual(len(buffer), 10*3220)
+
+
+    def test__truncate_padding_record_odd_correct(self):
+        """Test that truncate_padding_record is correctly dropping end padding"""
+        # Even number of scans, correct file length
+        reader = self.reader
+        # Odd number of scans, correct file length (should truncate)
+        reader.head = {'number_of_scans': 9}
+        buffer = reader._truncate_padding_record(bytes(10*3220))
+        self.assertEqual(len(buffer), 9*3220)
+
+
+    def test__truncate_padding_record_odd_nopadding(self):
+        """Test that truncate_padding_record is correctly dropping end padding"""
+        # Even number of scans, correct file length
+        reader = self.reader
+
+        # Odd number of scans, padding record is missing
+        with self.assertWarnsRegex(RuntimeWarning, "incomplete physical record"):
+            reader.head = {'number_of_scans': 9}
+            buffer = reader._truncate_padding_record(bytes(9*3220))
+            self.assertEqual(len(buffer), 9*3220)
+
+    def test__truncate_padding_record_shortfile(self):
+        """Test that truncate_padding_record is correctly dropping end padding"""
+        # Even number of scans, correct file length
+        reader = self.reader
+
+        # File is too short (should do nothing)
+        reader.head = {'number_of_scans': 9}
+        buffer = reader._truncate_padding_record(bytes(8*3220))
+        self.assertEqual(len(buffer), 8*3220)
+
+    def test__truncate_padding_record_longfile(self):
+        """Test that truncate_padding_record is correctly dropping end padding"""
+        # Even number of scans, correct file length
+        reader = self.reader
+
+        # File is too long (should do nothing)
+        reader.head = {'number_of_scans': 9}
+        buffer = reader._truncate_padding_record(bytes(12*3220))
+        self.assertEqual(len(buffer), 12*3220)