Repository: dirac-institute/sorcha

Issue:
Input files having >1 extra empty rows at the end breaks CSVReader
If an input file has an 2 or more blank lines after data rows CSVReader breaks and can't parse the extra skiprows function. Possibly need to make users aware to format input files correctly?

Hints from issue comments:
@astronomerritt: Saving the traceback here:

```
Traceback (most recent call last):
  File "/opt/miniconda3/envs/sorcha/bin/sorcha-run", line 8, in <module>
    sys.exit(main())
  File "/Users/stephaniemerritt/Projects/sorcha/src/sorcha_cmdline/run.py", line 121, in main
    return execute(args)
  File "/Users/stephaniemerritt/Projects/sorcha/src/sorcha_cmdline/run.py", line 183, in execute
    runLSSTSimulation(args, configs)
  File "/Users/stephaniemerritt/Projects/sorcha/src/sorcha/sorcha.py", line 190, in runLSSTSimulation
    orbits_df = reader.read_aux_block(block_size=configs["size_serial_chunk"])
  File "/Users/stephaniemerritt/Projects/sorcha/src/sorcha/readers/CombinedDataReader.py", line 241, in read_aux_block
    current_df = reader.read_objects(obj_ids)
  File "/Users/stephaniemerritt/Projects/sorcha/src/sorcha/readers/ObjectDataReader.py", line 125, in read_objects
    res_df = self._read_objects_internal(obj_ids, **kwargs)
  File "/Users/stephaniemerritt/Projects/sorcha/src/sorcha/readers/CSVReader.py", line 229, in _read_objects_internal
    res_df = pd.read_csv(
  File "/opt/miniconda3/envs/sorcha/lib/python3.10/site-packages/pandas/io/parsers/readers.py", line 1026, in read_csv
    return _read(filepath_or_buffer, kwds)
  File "/opt/miniconda3/envs/sorcha/lib/python3.10/site-packages/pandas/io/parsers/readers.py", line 626, in _read
    return parser.read(nrows)
  File "/opt/miniconda3/envs/sorcha/lib/python3.10/site-packages/pandas/io/parsers/readers.py", line 1923, in read
    ) = self._engine.read(  # type: ignore[attr-defined]
  File "/opt/miniconda3/envs/sorcha/lib/python3.10/site-packages/pandas/io/parsers/c_parser_wrapper.py", line 234, in read
    chunks = self._reader.read_low_memory(nrows)
  File "parsers.pyx", line 838, in pandas._libs.parsers.TextReader.read_low_memory
  File "parsers.pyx", line 905, in pandas._libs.parsers.TextReader._read_rows
  File "parsers.pyx", line 874, in pandas._libs.parsers.TextReader._tokenize_rows
  File "parsers.pyx", line 891, in pandas._libs.parsers.TextReader._check_tokenize_status
  File "parsers.pyx", line 2050, in pandas._libs.parsers.raise_parser_error
IndexError: list index out of range
Error: Command 'sorcha-run' failed with exit code 1.
```@astronomerritt: This is a bit of a pain, because the code is breaking BEFORE it does the validation checks on the input table. See lines 125-126 of ObjectDataReader.py:

```
res_df = self._read_objects_internal(obj_ids, **kwargs)  # code breaks here
res_df = self._process_and_validate_input_table(res_df, **kwargs)
```

My only suggestion for fixing this is to wrap one of the failing lines in a try/except IndexError statement and throw an error message that suggests the user might want to check for empty lines at the end of their input files.

I'd suggest doing this to the line I highlighted above, as it's in the parent class so all sub-classes would inherit the behaviour.@mschwamb: Can you check whether this is fixed by PR #1042 ?@mschwamb: @astronomerritt  ☝️ @astronomerritt: It is not fixed by that PR, no, sorry!

It's specifically the `_read_objects_internal()` methods on the reader classes causing the problem - even more specifically, it's something about either the skiprows argument on the Pandas `read_csv()` call or the way it's being utilised.

I _can_ stop the code from breaking entirely by adding `skip_blank_lines=True` to the Pandas `read_csv()` calls, but:

- this means the user is getting away with bad behaviour, as the code would no longer fail if there were ANY random blank lines in their input files
- I haven't fully checked for unexpected behaviour

The other option is to do what I suggested above, I guess, unless anyone has a better idea.@jeremykubica: @astronomerritt can you share the data file on which this is failing? I created a fake data file with blank lines and it is not giving me a problem.

Solution patch:
diff --git a/src/sorcha/readers/CSVReader.py b/src/sorcha/readers/CSVReader.py
index e74e8b3..9e633a0 100755
--- a/src/sorcha/readers/CSVReader.py
+++ b/src/sorcha/readers/CSVReader.py
@@ -40,6 +40,7 @@ class CSVDataReader(ObjectDataReader):
             sys.exit(f"ERROR: Unrecognized delimiter ({sep})")
         self.sep = sep
 
+        # To pre-validation and collect the header information.
         self.header_row = self._find_and_validate_header_line(header)
 
         # A table holding just the object ID for each row. Only populated
@@ -131,6 +132,37 @@ class CSVDataReader(ObjectDataReader):
             pplogger.error(error_str)
             sys.exit(error_str)
 
+    def _validate_csv(self, header):
+        """Perform a validation of the CSV file, such as checking for blank lines.
+
+        This is an expensive test and should only be performed when something
+        has gone wrong.  This is needed because panda's read_csv() function can
+        given vague errors (such as failing with an index error if the file
+        has blank lines at the end).
+
+        Parameters
+        ----------
+        header : integer
+            The row number of the header.
+
+        Returns
+        -------
+        : bool
+            True indicating success.
+        """
+        pplogger = logging.getLogger(__name__)
+
+        with open(self.filename) as fh:
+            for i, line in enumerate(fh):
+                if i >= header:
+                    # Check for blank lines. We do this explicitly because pandas read_csv()
+                    # has problems when skipping lines and finding blank lines at the end.
+                    if len(line) == 0 or line.isspace():
+                        error_str = f"ERROR: CSVReader: found a blank line on line {i} of {self.filename}."
+                        pplogger.error(error_str)
+                        sys.exit(error_str)
+        return True
+
     def _read_rows_internal(self, block_start=0, block_size=None, **kwargs):
         """Reads in a set number of rows from the input.
 
@@ -177,6 +209,7 @@ class CSVDataReader(ObjectDataReader):
                 skiprows=skip_rows,
                 nrows=block_size,
             )
+
         return res_df
 
     def _build_id_map(self):
@@ -225,18 +258,25 @@ class CSVDataReader(ObjectDataReader):
         skipped_row.extend(~self.obj_id_table["ObjID"].isin(obj_ids).values)
 
         # Read the rows.
-        if self.sep == "whitespace":
-            res_df = pd.read_csv(
-                self.filename,
-                sep="\\s+",
-                skiprows=(lambda x: skipped_row[x]),
-            )
-        else:
-            res_df = pd.read_csv(
-                self.filename,
-                delimiter=",",
-                skiprows=(lambda x: skipped_row[x]),
-            )
+        try:
+            if self.sep == "whitespace":
+                res_df = pd.read_csv(
+                    self.filename,
+                    sep="\\s+",
+                    skiprows=(lambda x: skipped_row[x]),
+                )
+            else:
+                res_df = pd.read_csv(
+                    self.filename,
+                    delimiter=",",
+                    skiprows=(lambda x: skipped_row[x]),
+                )
+        except IndexError as current_exc:
+            # Check if there is a more understandable error we can raise.
+            self._validate_csv(self.header_row)
+
+            # If we do not detect a problem with _validate_csv, reraise the error.
+            raise current_exc
         return res_df
 
     def _process_and_validate_input_table(self, input_table, **kwargs):


Test patch:
diff --git a/tests/readers/test_CSVReader.py b/tests/readers/test_CSVReader.py
index 19b2309..42fb0da 100755
--- a/tests/readers/test_CSVReader.py
+++ b/tests/readers/test_CSVReader.py
@@ -1,8 +1,10 @@
 import numpy as np
+import os
 import pandas as pd
 import pytest
 from numpy.testing import assert_equal
 from pandas.testing import assert_frame_equal
+import tempfile
 
 from sorcha.readers.CSVReader import CSVDataReader
 from sorcha.utilities.dataUtilitiesForTests import get_test_filepath
@@ -392,3 +394,30 @@ def test_CSVDataReader_delims():
     with pytest.raises(SystemExit) as e2:
         _ = CSVDataReader(get_test_filepath("testcolour.txt"), "")
     assert e2.type == SystemExit
+
+
+def test_CSVDataReader_blank_lines():
+    """Test that we fail if the input file has blank lines."""
+    with tempfile.TemporaryDirectory() as dir_name:
+        file_name = os.path.join(dir_name, "test.ecsv")
+        with open(file_name, "w") as output:
+            output.write("# My comment\n")
+            output.write("ObjID,b,c\n")
+            output.write("0,1,2\n")
+            output.write("1,1,2\n")
+            output.write("2,1,2\n")
+
+        # The checks pass.
+        reader = CSVDataReader(file_name, sep="csv", cache_table=False)
+        data = reader.read_objects(["1", "2"])
+        assert len(data) == 2
+
+        with open(file_name, "a") as output:
+            output.write("3,1,2\n")
+            output.write("\n")  # add another blank line
+            output.write("\n")  # add another blank line
+
+        # The code now fails by default.
+        reader2 = CSVDataReader(file_name, sep="csv", cache_table=False)
+        with pytest.raises(SystemExit):
+            _ = reader2.read_objects(["1", "2"])
diff --git a/tests/readers/test_OrbitAuxReader.py b/tests/readers/test_OrbitAuxReader.py
index 2938e4d..550415a 100755
--- a/tests/readers/test_OrbitAuxReader.py
+++ b/tests/readers/test_OrbitAuxReader.py
@@ -307,6 +307,7 @@ def test_orbit_reader_wrong_delim():
     with pytest.raises(SystemExit) as err:
         _ = OrbitAuxReader(get_test_filepath("testorb.des"), "csv")
 
+
 def test_orbit_sanity_check_kep():
     """If an orbit parameter is undefined, raise an exception"""
 
@@ -335,7 +336,6 @@ def test_orbit_sanity_check_kep():
     assert err.value.code == "ERROR: Invalid Keplerian elements detected for one or more objects (check log for information)"
 
 
-
 def test_orbit_sanity_check_com():
     """If an orbit parameter is undefined, raise an exception"""
 
@@ -350,4 +350,3 @@ def test_orbit_sanity_check_com():
         _ = OrbitAuxReader(get_test_filepath("orbit_test_files/orbit_sanity_check_com_e<0.csv"),"csv")
         _.read_rows()
     assert err.value.code == "ERROR: Invalid cometary elements detected for one or more objects (check log for information)"
-