From 3b268bdd19d743646a1229f5864f4bd47cbcce64 Mon Sep 17 00:00:00 2001 From: David Baines Date: Sat, 11 Jul 2026 14:48:06 +0100 Subject: [PATCH 1/3] Fix edge-case bugs in combine_scores aggregation - Skip empty scores CSVs instead of crashing on rows[0] (IndexError) - Strip header whitespace at read time so column lookups stay consistent with is_current_style and don't silently drop score columns in merge - Guard old-style rows against ragged/blank lines before DataFrame build - Use a fixed-depth glob (*/*/scores-*.csv) so series/experiment are read from the right path parts instead of being mislabeled at other depths - Raise a clear error in check_for_lock_file for unsupported file types instead of an UnboundLocalError Co-Authored-By: Claude Opus 4.8 (1M context) --- silnlp/common/combine_scores.py | 57 ++++++++++++++++++++------------- 1 file changed, 35 insertions(+), 22 deletions(-) diff --git a/silnlp/common/combine_scores.py b/silnlp/common/combine_scores.py index bf6187b3..b7b7b223 100644 --- a/silnlp/common/combine_scores.py +++ b/silnlp/common/combine_scores.py @@ -82,6 +82,8 @@ def check_for_lock_file(folder: Path, filename: str, file_type: str): lockfile = folder / f".~lock.{filename}.{file_type}#" elif file_type.lower() == "xlsx": lockfile = folder / f"~${filename}.{file_type}" + else: + raise ValueError(f"Unsupported file_type for lock file check: {file_type}") if lockfile.is_file(): print(f"Found lock file: {lockfile}") @@ -114,8 +116,10 @@ def aggregate_scores(folder): # Dictionary to store rows by header type data_by_header = defaultdict(list) - # Iterate over all CSV files in the folder and its subfolders - csv_files = list(folder.rglob("*/scores-*.csv")) + # Iterate over the scores files, which live at folder///scores-*.csv. + # Use a fixed-depth glob so parts[-3]/parts[-2] reliably map to series/experiment + # (a recursive rglob would match other depths and mislabel these fields). + csv_files = list(folder.glob("*/*/scores-*.csv")) for csv_file in csv_files: print(csv_file) if not csv_files: @@ -128,28 +132,37 @@ def aggregate_scores(folder): steps = csv_file.stem.split("-")[-1] # Extract steps from file name # Read the CSV file and add new columns - with open(csv_file, "r") as f: + with open(csv_file, "r", newline="") as f: reader = csv.reader(f) rows = list(reader) - header = list(rows[0]) # Use list for easier manipulation - - # Add columns to the beginning of each row - print(f"Processing {csv_file}") - if is_current_style(header): - # Transform header and rows for current style - transformed_header, transformed_rows = transform_current_style_rows(header, rows[1:]) - # Add Series, Experiment, Steps to the beginning - new_header = ["Series", "Experiment", "Steps"] + transformed_header - if tuple(new_header) not in data_by_header: - data_by_header[tuple(new_header)].append(new_header) - for row in transformed_rows: - data_by_header[tuple(new_header)].append([series, experiment, steps] + row) - else: - # Old style: keep as is - if tuple(header) not in data_by_header: - data_by_header[tuple(header)].append(["Series", "Experiment", "Steps"] + header) - for row in rows[1:]: - data_by_header[tuple(header)].append([series, experiment, steps] + row) + + if not rows: + print(f"Skipping empty file {csv_file}") + continue + + # Strip surrounding whitespace so column lookups stay consistent with is_current_style, + # which also strips before matching. + header = [col.strip() for col in rows[0]] + + # Add columns to the beginning of each row + print(f"Processing {csv_file}") + if is_current_style(header): + # Transform header and rows for current style + transformed_header, transformed_rows = transform_current_style_rows(header, rows[1:]) + # Add Series, Experiment, Steps to the beginning + new_header = ["Series", "Experiment", "Steps"] + transformed_header + if tuple(new_header) not in data_by_header: + data_by_header[tuple(new_header)].append(new_header) + for row in transformed_rows: + data_by_header[tuple(new_header)].append([series, experiment, steps] + row) + else: + # Old style: keep as is + if tuple(header) not in data_by_header: + data_by_header[tuple(header)].append(["Series", "Experiment", "Steps"] + header) + for row in rows[1:]: + if len(row) < len(header): + continue # skip incomplete or blank rows + data_by_header[tuple(header)].append([series, experiment, steps] + row) return data_by_header From 107cc634495d43fc25920611a6704819b8dffa0c Mon Sep 17 00:00:00 2001 From: David Baines Date: Mon, 13 Jul 2026 15:42:36 +0100 Subject: [PATCH 2/3] Simplify combine_scores with pandas read_csv and fix ALL-row filter Read each scores file directly with pd.read_csv(skipinitialspace=True) and concat/reindex to the canonical column set, replacing the manual csv.reader + list-of-lists + defaultdict layer. - Fix: clean_dataframe now strips string columns before filtering, so space-padded " ALL" rows with no chrF3++ score are dropped as intended (previously they survived because the filter compared against raw, un-stripped values on comma-space separated files). - Remove dead current-style machinery (is_current_style, transform_current_style_rows, CURRENT_STYLE_OUTPUT_COLUMNS): the detection could never return True because CURRENT_STYLE_COLUMNS lists the Series/Experiment/Steps columns this script itself synthesizes. - skipinitialspace removes the need for manual header/value stripping and eliminates the whitespace hazard at its root; on_bad_lines="warn" skips ragged rows instead of crashing, and EmptyDataError skips empty files. - Add type hints and derive numeric/string column sets from named lists. Co-Authored-By: Claude Opus 4.8 (1M context) --- silnlp/common/combine_scores.py | 513 ++++++++++++++------------------ 1 file changed, 225 insertions(+), 288 deletions(-) diff --git a/silnlp/common/combine_scores.py b/silnlp/common/combine_scores.py index b7b7b223..67c9114c 100644 --- a/silnlp/common/combine_scores.py +++ b/silnlp/common/combine_scores.py @@ -1,288 +1,225 @@ -import argparse -import csv -import sys -from collections import defaultdict -from pathlib import Path - -import openpyxl -import pandas as pd - -from ..common.environment import SilNlpEnv - -# Columns for current style detection and transformation (match actual input files) -CURRENT_STYLE_COLUMNS = [ - "Series", - "Experiment", - "Steps", - "book", - "draft_index", - "src_iso", - "trg_iso", - "num_refs", - "references", - "sent_len", - "BLEU", - "BLEU_1gram_prec", - "BLEU_2gram_prec", - "BLEU_3gram_prec", - "BLEU_4gram_prec", - "BLEU_brevity_penalty", - "BLEU_total_sys_len", - "BLEU_total_ref_len", - "chrF3", - "chrF3+", - "chrF3++", - "spBLEU", - "confidence", -] - -# Columns to hide in Excel output -COLUMNS_TO_HIDE = [ - "BLEU_1gram_prec", - "BLEU_2gram_prec", - "BLEU_3gram_prec", - "BLEU_4gram_prec", - "BLEU_brevity_penalty", - "BLEU_total_sys_len", - "BLEU_total_ref_len", - "chrF3", - "chrF3+", - "book", - "draft_index", - "num_refs", - "references", - "sent_len", - "spBLEU", - "confidence", -] - -# Final column order for current style -CURRENT_STYLE_OUTPUT_COLUMNS = [ - "src_iso", - "trg_iso", - "BLEU", - "chrF3++", - "book", - "draft_index", - "num_refs", - "references", - "sent_len", - "spBLEU", - "confidence", -] - - -def check_for_lock_file(folder: Path, filename: str, file_type: str): - """Check for lock files and ask the user to close them then exit.""" - - if file_type[0] == ".": - file_type = file_type[1:] - - if file_type.lower() == "csv": - lockfile = folder / f".~lock.{filename}.{file_type}#" - elif file_type.lower() == "xlsx": - lockfile = folder / f"~${filename}.{file_type}" - else: - raise ValueError(f"Unsupported file_type for lock file check: {file_type}") - - if lockfile.is_file(): - print(f"Found lock file: {lockfile}") - print(f"Please close {filename}.{file_type} in folder {folder} OR delete the lock file and try again.") - sys.exit() - - -def is_current_style(header): - """Check if the header matches the current style (all columns present, additional columns accepted).""" - header = [col.strip() for col in header] - return set(CURRENT_STYLE_COLUMNS).issubset(set(header)) - - -def transform_current_style_rows(header, rows): - """Remove and reorder columns for current style.""" - # Map column name to index - col_idx = {col: i for i, col in enumerate(header)} - # Only keep columns in CURRENT_STYLE_OUTPUT_COLUMNS - new_header = [col for col in CURRENT_STYLE_OUTPUT_COLUMNS if col in col_idx] - new_rows = [] - for row in rows: - if len(row) < len(header): - continue # skip incomplete or blank rows - new_row = [row[col_idx[col]] for col in new_header] - new_rows.append(new_row) - return new_header, new_rows - - -def aggregate_scores(folder): - # Dictionary to store rows by header type - data_by_header = defaultdict(list) - - # Iterate over the scores files, which live at folder///scores-*.csv. - # Use a fixed-depth glob so parts[-3]/parts[-2] reliably map to series/experiment - # (a recursive rglob would match other depths and mislabel these fields). - csv_files = list(folder.glob("*/*/scores-*.csv")) - for csv_file in csv_files: - print(csv_file) - if not csv_files: - print(f"No scores csv files were found in folder {folder.resolve()}") - sys.exit(0) - - for csv_file in csv_files: - series = csv_file.parts[-3] # Extract series folder name - experiment = csv_file.parts[-2] # Extract experiment folder name - steps = csv_file.stem.split("-")[-1] # Extract steps from file name - - # Read the CSV file and add new columns - with open(csv_file, "r", newline="") as f: - reader = csv.reader(f) - rows = list(reader) - - if not rows: - print(f"Skipping empty file {csv_file}") - continue - - # Strip surrounding whitespace so column lookups stay consistent with is_current_style, - # which also strips before matching. - header = [col.strip() for col in rows[0]] - - # Add columns to the beginning of each row - print(f"Processing {csv_file}") - if is_current_style(header): - # Transform header and rows for current style - transformed_header, transformed_rows = transform_current_style_rows(header, rows[1:]) - # Add Series, Experiment, Steps to the beginning - new_header = ["Series", "Experiment", "Steps"] + transformed_header - if tuple(new_header) not in data_by_header: - data_by_header[tuple(new_header)].append(new_header) - for row in transformed_rows: - data_by_header[tuple(new_header)].append([series, experiment, steps] + row) - else: - # Old style: keep as is - if tuple(header) not in data_by_header: - data_by_header[tuple(header)].append(["Series", "Experiment", "Steps"] + header) - for row in rows[1:]: - if len(row) < len(header): - continue # skip incomplete or blank rows - data_by_header[tuple(header)].append([series, experiment, steps] + row) - - return data_by_header - - -def clean_dataframe(df): - cleaned_df = df.copy() - cleaned_df = cleaned_df[ - ~((cleaned_df["trg_iso"] == "ALL") & (cleaned_df["chrF3++"].isna() | (cleaned_df["chrF3++"] == ""))) - ] - numeric_cols = [ - "BLEU", - "BLEU_1gram_prec", - "BLEU_2gram_prec", - "BLEU_3gram_prec", - "BLEU_4gram_prec", - "BLEU_brevity_penalty", - "BLEU_total_sys_len", - "BLEU_total_ref_len", - "chrF3", - "chrF3+", - "chrF3++", - "spBLEU", - "confidence", - "num_refs", - "sent_len", - "draft_index", - ] - for col in numeric_cols: - if col in cleaned_df.columns: - cleaned_df[col] = pd.to_numeric(cleaned_df[col], errors="coerce") - string_cols = ["Series", "Experiment", "Steps", "book", "src_iso", "trg_iso", "references"] - for col in string_cols: - if col in cleaned_df.columns: - cleaned_df[col] = cleaned_df[col].astype(str).str.strip() - return cleaned_df - - -def merge_all_data(data_by_header): - all_dfs = [] - for header, rows in data_by_header.items(): - if len(rows) > 1: - df = pd.DataFrame(rows[1:], columns=rows[0]) - all_dfs.append(df) - final_columns = CURRENT_STYLE_COLUMNS - if not all_dfs: - # Return empty DataFrame with expected columns so downstream code can access them safely - return pd.DataFrame(columns=final_columns) - combined = pd.concat(all_dfs, ignore_index=True, sort=False) - for col in final_columns: - if col not in combined.columns: - combined[col] = None - return combined[final_columns] - - -def sort_dataframe(df, sort_by): - sort_cols = [col for col, _ in sort_by] - sort_ascending = [asc for _, asc in sort_by] - return df.sort_values(by=sort_cols, ascending=sort_ascending, na_position="last") - - -def write_to_excel(df, folder, output_filename): - output_file = folder / f"{output_filename}.xlsx" - with pd.ExcelWriter(output_file, engine="openpyxl") as writer: - df.to_excel(writer, sheet_name="Scores", index=False) - wb = openpyxl.load_workbook(output_file) - ws = wb["Scores"] - header_row = next(ws.iter_rows(min_row=1, max_row=1, values_only=True)) - for col_idx, col_name in enumerate(header_row, 1): - col_letter = openpyxl.utils.get_column_letter(col_idx) - if col_name in COLUMNS_TO_HIDE: - ws.column_dimensions[col_letter].hidden = True - else: - max_length = len(str(col_name)) if col_name else 0 - for cell in ws[col_letter]: - if cell.row == 1: - continue - try: - cell_length = len(str(cell.value)) if cell.value is not None else 0 - if cell_length > max_length: - max_length = cell_length - except Exception: - pass - ws.column_dimensions[col_letter].width = max_length + 2 - wb.save(output_file) - print(f"Wrote scores to {output_file}") - - -def main(): - parser = argparse.ArgumentParser(description="Aggregate CSV files in a folder.") - parser.add_argument("folder", type=Path, help="Path to the folder containing CSV files.") - parser.add_argument( - "--output_filename", - type=str, - default="scores", - help="Filename suffix without the '.csv' or '.xlsx'. \ - The folder name is added as a prefix to make it easier to distinguish scores files in search results.", - ) - args = parser.parse_args() - - environment = SilNlpEnv.create_standard_environment() - folder = Path(args.folder) - base_filename = f"{folder.name}_{args.output_filename}" - - if not folder.is_dir(): - folder = Path(environment.mt_experiments_dir) / args.folder - - # Check for lock files and ask the user to close them. - check_for_lock_file(folder, base_filename, "xlsx") - - # Aggregate the data from all the scores files. - data = aggregate_scores(folder) - combined_df = merge_all_data(data) - - # Clean and sort the data - clean_df = clean_dataframe(combined_df) - sorted_df = sort_dataframe(clean_df, sort_by=[("Series", True), ("chrF3++", False), ("BLEU", False)]) - - # Write the data to an excel file - write_to_excel(sorted_df, folder, base_filename) - - -if __name__ == "__main__": - main() +import argparse +import sys +from pathlib import Path + +import openpyxl +import pandas as pd + +from ..common.environment import SilNlpEnv + +# Canonical column set and order for the combined output. Series/Experiment/Steps +# are synthesized by this script; the rest come from the input scores files. +CURRENT_STYLE_COLUMNS = [ + "Series", + "Experiment", + "Steps", + "book", + "draft_index", + "src_iso", + "trg_iso", + "num_refs", + "references", + "sent_len", + "BLEU", + "BLEU_1gram_prec", + "BLEU_2gram_prec", + "BLEU_3gram_prec", + "BLEU_4gram_prec", + "BLEU_brevity_penalty", + "BLEU_total_sys_len", + "BLEU_total_ref_len", + "chrF3", + "chrF3+", + "chrF3++", + "spBLEU", + "confidence", +] + +# Columns to hide in Excel output +COLUMNS_TO_HIDE = [ + "BLEU_1gram_prec", + "BLEU_2gram_prec", + "BLEU_3gram_prec", + "BLEU_4gram_prec", + "BLEU_brevity_penalty", + "BLEU_total_sys_len", + "BLEU_total_ref_len", + "chrF3", + "chrF3+", + "book", + "draft_index", + "num_refs", + "references", + "sent_len", + "spBLEU", + "confidence", +] + +# Columns that should be treated as numbers (for coercion and sorting). +NUMERIC_COLUMNS = [ + "BLEU", + "BLEU_1gram_prec", + "BLEU_2gram_prec", + "BLEU_3gram_prec", + "BLEU_4gram_prec", + "BLEU_brevity_penalty", + "BLEU_total_sys_len", + "BLEU_total_ref_len", + "chrF3", + "chrF3+", + "chrF3++", + "spBLEU", + "confidence", + "num_refs", + "sent_len", + "draft_index", +] + +# Columns that should be treated as free text. +STRING_COLUMNS = ["Series", "Experiment", "Steps", "book", "src_iso", "trg_iso", "references"] + + +def check_for_lock_file(folder: Path, filename: str, file_type: str) -> None: + """Check for lock files and ask the user to close them then exit.""" + + if file_type[0] == ".": + file_type = file_type[1:] + + if file_type.lower() == "csv": + lockfile = folder / f".~lock.{filename}.{file_type}#" + elif file_type.lower() == "xlsx": + lockfile = folder / f"~${filename}.{file_type}" + else: + raise ValueError(f"Unsupported file_type for lock file check: {file_type}") + + if lockfile.is_file(): + print(f"Found lock file: {lockfile}") + print(f"Please close {filename}.{file_type} in folder {folder} OR delete the lock file and try again.") + sys.exit() + + +def load_scores(folder: Path) -> pd.DataFrame: + """Read every folder///scores-*.csv file into one DataFrame. + + The fixed-depth glob guarantees parts[-3]/parts[-2] map to series/experiment. + ``skipinitialspace`` drops the leading blanks in the comma-space separated files, + and reading everything as strings preserves the raw values for later coercion. + """ + csv_files = sorted(folder.glob("*/*/scores-*.csv")) + if not csv_files: + print(f"No scores csv files were found in folder {folder.resolve()}") + sys.exit(0) + + frames = [] + for csv_file in csv_files: + print(f"Processing {csv_file}") + try: + df = pd.read_csv(csv_file, dtype=str, skipinitialspace=True, skip_blank_lines=True, on_bad_lines="warn") + except pd.errors.EmptyDataError: + print(f"Skipping empty file {csv_file}") + continue + + # Strip any whitespace left around header names so column lookups are reliable. + df.columns = df.columns.astype(str).str.strip() + df.insert(0, "Series", csv_file.parts[-3]) + df.insert(1, "Experiment", csv_file.parts[-2]) + df.insert(2, "Steps", csv_file.stem.split("-")[-1]) + frames.append(df) + + if not frames: + # Return an empty DataFrame with expected columns so downstream code is safe. + return pd.DataFrame(columns=CURRENT_STYLE_COLUMNS) + + combined = pd.concat(frames, ignore_index=True, sort=False) + # Normalize to the canonical column set/order; missing columns become NaN. + return combined.reindex(columns=CURRENT_STYLE_COLUMNS) + + +def clean_dataframe(df: pd.DataFrame) -> pd.DataFrame: + cleaned_df = df.copy() + + # Strip string columns BEFORE any value comparison so whitespace-padded values + # (e.g. " ALL") are matched correctly by the filter below. + for col in STRING_COLUMNS: + if col in cleaned_df.columns: + cleaned_df[col] = cleaned_df[col].astype(str).str.strip() + + # Rows with 'ALL' in trg_iso and no chrF3++ score are not useful, so drop them. + if "trg_iso" in cleaned_df.columns and "chrF3++" in cleaned_df.columns: + chrf3pp = cleaned_df["chrF3++"] + missing_chrf3pp = chrf3pp.isna() | (chrf3pp.astype(str).str.strip() == "") + cleaned_df = cleaned_df[~((cleaned_df["trg_iso"] == "ALL") & missing_chrf3pp)] + + for col in NUMERIC_COLUMNS: + if col in cleaned_df.columns: + cleaned_df[col] = pd.to_numeric(cleaned_df[col], errors="coerce") + + return cleaned_df + + +def sort_dataframe(df: pd.DataFrame, sort_by) -> pd.DataFrame: + sort_cols = [col for col, _ in sort_by] + sort_ascending = [asc for _, asc in sort_by] + return df.sort_values(by=sort_cols, ascending=sort_ascending, na_position="last") + + +def write_to_excel(df: pd.DataFrame, folder: Path, output_filename: str) -> None: + output_file = folder / f"{output_filename}.xlsx" + with pd.ExcelWriter(output_file, engine="openpyxl") as writer: + df.to_excel(writer, sheet_name="Scores", index=False) + wb = openpyxl.load_workbook(output_file) + ws = wb["Scores"] + header_row = next(ws.iter_rows(min_row=1, max_row=1, values_only=True)) + for col_idx, col_name in enumerate(header_row, 1): + col_letter = openpyxl.utils.get_column_letter(col_idx) + if col_name in COLUMNS_TO_HIDE: + ws.column_dimensions[col_letter].hidden = True + else: + max_length = len(str(col_name)) if col_name else 0 + for cell in ws[col_letter]: + if cell.row == 1: + continue + try: + cell_length = len(str(cell.value)) if cell.value is not None else 0 + if cell_length > max_length: + max_length = cell_length + except Exception: + pass + ws.column_dimensions[col_letter].width = max_length + 2 + wb.save(output_file) + print(f"Wrote scores to {output_file}") + + +def main() -> None: + parser = argparse.ArgumentParser(description="Aggregate CSV files in a folder.") + parser.add_argument("folder", type=Path, help="Path to the folder containing CSV files.") + parser.add_argument( + "--output_filename", + type=str, + default="scores", + help="Filename suffix without the '.csv' or '.xlsx'. \ + The folder name is added as a prefix to make it easier to distinguish scores files in search results.", + ) + args = parser.parse_args() + + environment = SilNlpEnv.create_standard_environment() + folder = Path(args.folder) + base_filename = f"{folder.name}_{args.output_filename}" + + if not folder.is_dir(): + folder = Path(environment.mt_experiments_dir) / args.folder + + # Check for lock files and ask the user to close them. + check_for_lock_file(folder, base_filename, "xlsx") + + # Read, clean and sort the scores. + combined_df = load_scores(folder) + clean_df = clean_dataframe(combined_df) + sorted_df = sort_dataframe(clean_df, sort_by=[("Series", True), ("chrF3++", False), ("BLEU", False)]) + + # Write the data to an excel file + write_to_excel(sorted_df, folder, base_filename) + + +if __name__ == "__main__": + main() From e30600e01c58ed630db1e2d2429c0b27968b0cc9 Mon Sep 17 00:00:00 2001 From: David Baines Date: Mon, 13 Jul 2026 16:22:26 +0100 Subject: [PATCH 3/3] Fix combine_scores glob depth regression Scores files live one level below the passed folder (folder//scores-*.csv), but the fixed-depth glob "*/*/scores-*.csv" required two levels and matched nothing. Restore the original rglob("*/scores-*.csv") so per-experiment subfolders (and any deeper nesting) are found; parts[-2]/parts[-3] remain the experiment and series directories. Co-Authored-By: Claude Opus 4.8 (1M context) --- silnlp/common/combine_scores.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/silnlp/common/combine_scores.py b/silnlp/common/combine_scores.py index 67c9114c..cfd9b920 100644 --- a/silnlp/common/combine_scores.py +++ b/silnlp/common/combine_scores.py @@ -101,11 +101,13 @@ def check_for_lock_file(folder: Path, filename: str, file_type: str) -> None: def load_scores(folder: Path) -> pd.DataFrame: """Read every folder///scores-*.csv file into one DataFrame. - The fixed-depth glob guarantees parts[-3]/parts[-2] map to series/experiment. + Scores files live in per-experiment subfolders (folder//scores-*.csv), + and may be nested more deeply, so recurse. parts[-2]/parts[-3] are the file's + immediate parent (experiment) and grandparent (series) directories. ``skipinitialspace`` drops the leading blanks in the comma-space separated files, and reading everything as strings preserves the raw values for later coercion. """ - csv_files = sorted(folder.glob("*/*/scores-*.csv")) + csv_files = sorted(folder.rglob("*/scores-*.csv")) if not csv_files: print(f"No scores csv files were found in folder {folder.resolve()}") sys.exit(0)