diff --git a/CHANGELOG.md b/CHANGELOG.md index cce6456a..c40b4a6b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -97,6 +97,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 and `backcheck_target_percent`, and the target is saved under `backcheck_target_percent` so it reloads in later sessions. Targets saved under the old `backcheck_goal` key were never applied and are ignored — #299 +- **Backcheck dates**: `_add_date_columns` joined backcheck dates on the survey + KEY, so when survey and backcheck KEYs differed the dates and the "Avg Days" + statistics were empty. Backcheck dates now join on the backcheck KEY + (`{survey_key}__BCCL`). Backchecker statistics were also always empty when + the survey KEY was the merge ID; they are now computed — #300 - **Correction log schema**: Removing the last correction entry now leaves an empty log with the full schema, including status columns — #296 - **Constraint violations**: A value past a hard bound was reported as a soft diff --git a/src/datasure/checks/backchecks/compute.py b/src/datasure/checks/backchecks/compute.py index d63a094c..fcde2265 100644 --- a/src/datasure/checks/backchecks/compute.py +++ b/src/datasure/checks/backchecks/compute.py @@ -8,10 +8,12 @@ from scipy import stats from datasure.checks.backchecks.models import ( + BACKCHECK_SUFFIX, TAB_NAME, WEEKDAY_OFFSET_TO_NUMERIC, BackcheckSettings, SearchType, + merged_backcheck_name, ) from datasure.utils.settings_utils import load_check_settings @@ -317,7 +319,7 @@ def _process_backcheck_column( if col not in merged_data.columns: return None - backcheck_col = f"{col}__BCCL" + backcheck_col = merged_backcheck_name(col) if backcheck_col not in merged_data.columns: return None @@ -453,7 +455,7 @@ def compute_backcheck_analysis( backcheck_for_merge, on=survey_id, how="inner", - suffix="__BCCL", + suffix=BACKCHECK_SUFFIX, ) if merged_data.is_empty(): @@ -627,8 +629,7 @@ def _get_staff_configuration( survey_data: pl.DataFrame, backcheck_data: pl.DataFrame, backcheck_settings: BackcheckSettings, - survey_key: str, -) -> tuple[str, pl.DataFrame, str] | None: +) -> tuple[str, pl.DataFrame] | None: """Get staff column configuration based on staff type. Parameters @@ -641,27 +642,66 @@ def _get_staff_configuration( Backcheck dataset. backcheck_settings : BackcheckSettings Backcheck settings. - survey_key : str - Survey key column name. Returns ------- - tuple[str, pl.DataFrame, str] | None - Tuple of (staff_col, data_source, join_key) if valid, None otherwise. + tuple[str, pl.DataFrame] | None + Tuple of (staff_col, data_source) if valid, None otherwise. """ if staff_type == "enumerator": staff_col = backcheck_settings.enumerator data_source = survey_data - join_key = survey_key else: # backchecker staff_col = backcheck_settings.backchecker data_source = backcheck_data - join_key = f"{survey_key}__BCCL" if not staff_col or staff_col not in data_source.columns: return None - return staff_col, data_source, join_key + return staff_col, data_source + + +def _join_backcheck_columns( + analysis: pl.DataFrame, + backcheck_data: pl.DataFrame, + survey_key: str, + columns: dict[str, str], +) -> pl.DataFrame: + """Left-join backcheck columns onto the analysis by backcheck KEY. + + The analysis holds the backcheck KEY as `{survey_key}__BCCL`. The merge only + adds that column when both datasets have `survey_key`; if `survey_key` is + also the merge ID it is shared, and the join uses `survey_key` as is. + + Parameters + ---------- + analysis : pl.DataFrame + Backcheck analysis results. + backcheck_data : pl.DataFrame + Backcheck dataset. + survey_key : str + Survey key column name. + columns : dict[str, str] + Backcheck columns to add, mapped to their names in the result. + + Returns + ------- + pl.DataFrame + Analysis with the columns added, or unchanged if the backcheck data + lacks `survey_key` or any of the columns. + """ + if any(col not in backcheck_data.columns for col in [survey_key, *columns]): + return analysis + + backcheck_key = merged_backcheck_name(survey_key) + if backcheck_key not in analysis.columns: + backcheck_key = survey_key + + backcheck_info = backcheck_data.select( + pl.col(survey_key).alias(backcheck_key), + *(pl.col(col).alias(alias) for col, alias in columns.items()), + ).unique(subset=[backcheck_key]) + return analysis.join(backcheck_info, on=backcheck_key, how="left") def _join_staff_information( @@ -669,7 +709,6 @@ def _join_staff_information( data_source: pl.DataFrame, staff_col: str, survey_key: str, - join_key: str, staff_type: str, ) -> pl.DataFrame: """Join backcheck analysis with staff information. @@ -684,8 +723,6 @@ def _join_staff_information( Staff column name. survey_key : str Survey key column name. - join_key : str - Key to join on. staff_type : str Either "enumerator" or "backchecker". @@ -694,14 +731,13 @@ def _join_staff_information( pl.DataFrame Analysis joined with staff information. """ - staff_info = data_source.select([survey_key, staff_col]).unique(subset=[survey_key]) - - if staff_type == "enumerator": - return backcheck_analysis.join(staff_info, on=survey_key, how="left") + if staff_type == "backchecker": + return _join_backcheck_columns( + backcheck_analysis, data_source, survey_key, {staff_col: staff_col} + ) - # For backcheckers, rename survey_key to match backcheck key - staff_info = staff_info.rename({survey_key: join_key}) - return backcheck_analysis.join(staff_info, on=join_key, how="left") + staff_info = data_source.select([survey_key, staff_col]).unique(subset=[survey_key]) + return backcheck_analysis.join(staff_info, on=survey_key, how="left") def _add_date_columns( @@ -744,11 +780,10 @@ def _add_date_columns( result = result.join(survey_dates, on=survey_key, how="left") # Add backcheck date - if backcheck_date and backcheck_date in backcheck_data.columns: - bc_dates = backcheck_data.select( - [survey_key, pl.col(backcheck_date).alias("backcheck_date_col")] - ).unique(subset=[survey_key]) - result = result.join(bc_dates, on=survey_key, how="left") + if backcheck_date: + result = _join_backcheck_columns( + result, backcheck_data, survey_key, {backcheck_date: "backcheck_date_col"} + ) return result @@ -970,21 +1005,20 @@ def compute_enumerator_backchecker_stats( # Get staff configuration staff_config = _get_staff_configuration( - staff_type, survey_data, backcheck_data, backcheck_settings, survey_key + staff_type, survey_data, backcheck_data, backcheck_settings ) if staff_config is None: return pl.DataFrame() - staff_col, data_source, join_key = staff_config - - # Check if join key exists in analysis - if join_key not in backcheck_analysis.columns: - return pl.DataFrame() + staff_col, data_source = staff_config # Join analysis with staff information analysis_with_staff = _join_staff_information( - backcheck_analysis, data_source, staff_col, survey_key, join_key, staff_type + backcheck_analysis, data_source, staff_col, survey_key, staff_type ) + # The backchecker join is skipped when the backcheck data has no survey_key + if staff_col not in analysis_with_staff.columns: + return pl.DataFrame() # Add date columns analysis_with_staff = _add_date_columns( @@ -1325,7 +1359,7 @@ def _build_select_columns( ] # Include backcheck key if it exists in the data - backcheck_key = f"{survey_key}__BCCL" + backcheck_key = merged_backcheck_name(survey_key) if backcheck_key in data.columns: select_cols.insert(1, pl.col(backcheck_key)) @@ -1436,8 +1470,8 @@ def _are_columns_numeric( bool True if both columns are numeric. """ - # Remove __BCCL suffix from backcheck column for schema lookup - backcheck_col_original = backcheck_col.replace("__BCCL", "") + # Remove the backcheck suffix from the column for schema lookup + backcheck_col_original = backcheck_col.removesuffix(BACKCHECK_SUFFIX) return ( data.schema[survey_col].is_numeric() and data.schema[backcheck_col_original].is_numeric() diff --git a/src/datasure/checks/backchecks/models.py b/src/datasure/checks/backchecks/models.py index 59704427..dbc7e9de 100644 --- a/src/datasure/checks/backchecks/models.py +++ b/src/datasure/checks/backchecks/models.py @@ -6,6 +6,16 @@ TAB_NAME: str = "backchecks" +# Suffix the survey/backcheck merge adds to backcheck columns whose names +# clash with survey columns, e.g. the backcheck KEY becomes "KEY__BCCL". +BACKCHECK_SUFFIX: str = "__BCCL" + + +def merged_backcheck_name(col: str) -> str: + """Return the merged-data name of backcheck column `col`.""" + return f"{col}{BACKCHECK_SUFFIX}" + + # Weekday constants for productivity analysis WEEKDAY_NAMES = [ "Monday", diff --git a/src/datasure/checks/backchecks/report_ui.py b/src/datasure/checks/backchecks/report_ui.py index 0e81dc44..a653fd69 100644 --- a/src/datasure/checks/backchecks/report_ui.py +++ b/src/datasure/checks/backchecks/report_ui.py @@ -22,6 +22,7 @@ OkRangeType, OkRangeValues, SearchType, + merged_backcheck_name, ) from datasure.checks.backchecks.settings_ui import backchecks_report_settings from datasure.utils.dataframe_utils import ColumnByType @@ -1447,7 +1448,7 @@ def _render_backcheck_comparison_results( # Extract settings survey_key = backcheck_settings.survey_key survey_id = backcheck_settings.survey_id - backcheck_key = f"{survey_key}__BCCL" + backcheck_key = merged_backcheck_name(survey_key) # Get available columns from backcheck_analysis available_columns = sorted( diff --git a/tests/checks/backchecks/test_compute.py b/tests/checks/backchecks/test_compute.py index 0940f042..ebf0fc51 100644 --- a/tests/checks/backchecks/test_compute.py +++ b/tests/checks/backchecks/test_compute.py @@ -27,6 +27,7 @@ _get_column_data_type, _get_staff_configuration, _get_test_value, + _join_backcheck_columns, _join_staff_information, _perform_statistical_tests, _prepare_data_for_merge, @@ -651,9 +652,7 @@ def test_compute_enumerator_backchecker_stats_backchecker( col_settings, ) - # Check that analysis has the backcheck key column assert not analysis.is_empty() - backcheck_key = f"{sample_backcheck_settings.survey_key}__BCCL" result = compute_enumerator_backchecker_stats( sample_survey_data_pl, @@ -663,14 +662,8 @@ def test_compute_enumerator_backchecker_stats_backchecker( "backchecker", ) - # The result might be empty if the backcheck key is not properly set up - # Check if backcheck key exists in analysis before asserting non-empty result - if backcheck_key in analysis.columns: - assert not result.is_empty() - assert "backchecker" in result.columns - else: - # If backcheck key is not in analysis, the result will be empty - assert result.is_empty() + assert not result.is_empty() + assert "backchecker" in result.columns def test_compute_enumerator_backchecker_stats_empty_analysis( @@ -2011,13 +2004,12 @@ def test_get_staff_configuration_enumerator( sample_survey_data_pl, sample_backcheck_data_pl, sample_backcheck_settings, - "survey_id", ) assert result is not None - staff_col, _, join_key = result + staff_col, data_source = result assert staff_col == "enumerator" - assert join_key == "survey_id" + assert data_source is sample_survey_data_pl def test_get_staff_configuration_backchecker( @@ -2029,13 +2021,12 @@ def test_get_staff_configuration_backchecker( sample_survey_data_pl, sample_backcheck_data_pl, sample_backcheck_settings, - "survey_id", ) assert result is not None - staff_col, _, join_key = result + staff_col, data_source = result assert staff_col == "backchecker" - assert join_key == "survey_id__BCCL" + assert data_source is sample_backcheck_data_pl def test_join_staff_information(): @@ -2054,13 +2045,23 @@ def test_join_staff_information(): ) result = _join_staff_information( - analysis, survey_data, "staff", "survey_id", "survey_id", "enumerator" + analysis, survey_data, "staff", "survey_id", "enumerator" ) assert "staff" in result.columns assert len(result) == 2 +def test_join_backcheck_columns_survey_key_is_merge_id(): + """With no backcheck KEY column in the analysis, join on the shared survey_key.""" + analysis = pl.DataFrame({"sid": ["A", "B"], "column_name": ["age", "age"]}) + backcheck_data = pl.DataFrame({"sid": ["A", "B"], "bcer": ["B1", "B2"]}) + + result = _join_backcheck_columns(analysis, backcheck_data, "sid", {"bcer": "bcer"}) + + assert result["bcer"].to_list() == ["B1", "B2"] + + def test_add_date_columns(): """Test _add_date_columns.""" analysis = pl.DataFrame( @@ -2095,6 +2096,183 @@ def test_add_date_columns(): assert "backcheck_date_col" in result.columns +def test_add_date_columns_distinct_survey_and_backcheck_keys(): + """Backcheck dates join on the backcheck KEY when it differs from the survey KEY.""" + analysis = pl.DataFrame( + { + "key": ["s-1", "s-2"], + "key__BCCL": ["b-1", "b-2"], + "column_name": ["age", "age"], + } + ) + survey_data = pl.DataFrame( + { + "key": ["s-1", "s-2"], + "survey_date": [date(2024, 1, 1), date(2024, 1, 2)], + } + ) + backcheck_data = pl.DataFrame( + { + "key": ["b-1", "b-2"], + "backcheck_date": [date(2024, 1, 5), date(2024, 1, 9)], + } + ) + + result = _add_date_columns( + analysis, survey_data, backcheck_data, "key", "survey_date", "backcheck_date" + ) + + assert result["backcheck_date_col"].to_list() == [ + date(2024, 1, 5), + date(2024, 1, 9), + ] + assert _calculate_average_days(result, "survey_date", "backcheck_date") == 5.5 + + +def test_add_date_columns_backcheck_data_without_key(): + """Backcheck dates are skipped, not a crash, when backcheck data has no KEY.""" + analysis = pl.DataFrame({"key": ["s-1"], "column_name": ["age"]}) + survey_data = pl.DataFrame({"key": ["s-1"], "survey_date": [date(2024, 1, 1)]}) + backcheck_data = pl.DataFrame({"sid": ["A"], "backcheck_date": [date(2024, 1, 5)]}) + + result = _add_date_columns( + analysis, survey_data, backcheck_data, "key", "survey_date", "backcheck_date" + ) + + assert "survey_date_col" in result.columns + assert "backcheck_date_col" not in result.columns + + +@pytest.mark.parametrize("staff_type", ["enumerator", "backchecker"]) +def test_stats_avg_days_with_distinct_survey_and_backcheck_keys(staff_type): + """Avg Days uses each backcheck's own date when the two datasets' KEYs differ.""" + survey_data = pl.DataFrame( + { + "key": ["s-1", "s-2"], + "sid": ["A", "B"], + "enum": ["E1", "E1"], + "sdate": [date(2024, 1, 1), date(2024, 1, 1)], + "age": [25, 30], + } + ) + backcheck_data = pl.DataFrame( + { + "key": ["b-1", "b-2"], + "sid": ["A", "B"], + "bcer": ["B1", "B1"], + "bdate": [date(2024, 1, 3), date(2024, 1, 5)], + "age": [25, 31], + } + ) + settings = BackcheckSettings( + survey_key="key", + survey_id="sid", + survey_date="sdate", + backcheck_date="bdate", + enumerator="enum", + backchecker="bcer", + ) + col_settings = pl.DataFrame( + { + "search_type": ["exact"], + "pattern": ["age"], + "column_name": [["age"]], + "category": [1], + "ok_range_type": [None], + "ok_range_values": [None], + "ttest": [False], + "prtest": [False], + "signrank": [False], + "reliability": [False], + } + ) + analysis = compute_backcheck_analysis( + survey_data, backcheck_data, settings, col_settings + ) + + result = compute_enumerator_backchecker_stats( + survey_data, backcheck_data, analysis, settings, staff_type + ) + + assert result["Avg Days"].to_list() == [3.0] + + +def _age_column_settings() -> pl.DataFrame: + return pl.DataFrame( + { + "search_type": ["exact"], + "pattern": ["age"], + "column_name": [["age"]], + "category": [1], + "ok_range_type": [None], + "ok_range_values": [None], + "ttest": [False], + "prtest": [False], + "signrank": [False], + "reliability": [False], + } + ) + + +def test_backchecker_stats_when_survey_key_is_merge_id(): + """Backchecker stats are computed when survey_key is also the merge ID.""" + survey_data = pl.DataFrame( + { + "sid": ["A", "B"], + "enum": ["E1", "E1"], + "sdate": [date(2024, 1, 1), date(2024, 1, 1)], + "age": [25, 30], + } + ) + backcheck_data = pl.DataFrame( + { + "sid": ["A", "B"], + "bcer": ["B1", "B1"], + "bdate": [date(2024, 1, 3), date(2024, 1, 5)], + "age": [25, 31], + } + ) + settings = BackcheckSettings( + survey_key="sid", + survey_id="sid", + survey_date="sdate", + backcheck_date="bdate", + enumerator="enum", + backchecker="bcer", + ) + analysis = compute_backcheck_analysis( + survey_data, backcheck_data, settings, _age_column_settings() + ) + + result = compute_enumerator_backchecker_stats( + survey_data, backcheck_data, analysis, settings, "backchecker" + ) + + assert result["bcer"].to_list() == ["B1"] + assert result["Mismatches (Total)"].to_list() == [1] + assert result["Avg Days"].to_list() == [3.0] + + +def test_backchecker_stats_empty_when_backcheck_data_has_no_survey_key(): + """Backchecker stats are empty when the backcheck data lacks survey_key.""" + survey_data = pl.DataFrame( + {"key": ["s-1", "s-2"], "sid": ["A", "B"], "age": [25, 30]} + ) + backcheck_data = pl.DataFrame( + {"sid": ["A", "B"], "bcer": ["B1", "B1"], "age": [25, 31]} + ) + settings = BackcheckSettings(survey_key="key", survey_id="sid", backchecker="bcer") + analysis = compute_backcheck_analysis( + survey_data, backcheck_data, settings, _age_column_settings() + ) + + result = compute_enumerator_backchecker_stats( + survey_data, backcheck_data, analysis, settings, "backchecker" + ) + + assert result.is_empty() + + def test_calculate_average_days(): """Test _calculate_average_days.""" staff_data = pl.DataFrame( @@ -2517,7 +2695,6 @@ def test_get_staff_configuration_missing_staff_col( sample_survey_data_pl, sample_backcheck_data_pl, settings, - "survey_id", ) assert result is None @@ -2655,15 +2832,15 @@ def test_process_backcheck_column_backcheck_col_missing(): def test_join_staff_information_backchecker_path(): - """_join_staff_information renames survey_key to join_key for backcheckers.""" - analysis = pl.DataFrame({"key": [1, 2], "bc_key": [10, 20]}) - data_source = pl.DataFrame({"key": [1, 2], "staff": ["a", "b"]}) + """_join_staff_information joins backcheckers on the backcheck KEY.""" + analysis = pl.DataFrame({"key": [1, 2], "key__BCCL": [10, 20]}) + data_source = pl.DataFrame({"key": [20, 10], "staff": ["b", "a"]}) result = _join_staff_information( - analysis, data_source, "staff", "key", "bc_key", "backchecker" + analysis, data_source, "staff", "key", "backchecker" ) - assert "staff" in result.columns + assert result["staff"].to_list() == ["a", "b"] def test_add_date_columns_no_dates():