From 580429cee53dd093f4b033c38f8b5d65e7531f85 Mon Sep 17 00:00:00 2001 From: iabaako Date: Thu, 1 Oct 2026 10:53:13 +0000 Subject: [PATCH 01/12] feat(outliers): correct or accept outliers and constraint violations from the check page - Selecting a row in the constraint violations or outlier inspection table opens the shared correction form, prefilled with the row's KEY, column and current value: modify value, remove value or accept as valid. Entries are logged with source and check type `outliers` or `constraints` - Accepted flags are hidden from the tables and left out of the metrics; a "Show reviewed" toggle shows them with a Reviewed badge and the reason. They come back when the value changes or the acceptance is removed. Outlier and constraint acceptances are independent - Accepting a hard constraint violation needs a confirmation and is logged with a new `severity` column ("hard"), highlighted in the Correction Log - After a save the page reruns, the table selection resets, and a toast plus a page link point to the Correction Log - Flag review logic lives in the Streamlit-free checks/outliers/review.py - outliers_report takes the dataset alias; remove unused _render_outlier_table Refs #298 Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 14 + docs/USER_GUIDE.md | 21 + src/datasure/checks/outliers/report_ui.py | 333 ++++++++++--- src/datasure/checks/outliers/review.py | 204 ++++++++ src/datasure/processing/correction_log.py | 7 + src/datasure/processing/corrections.py | 14 + src/datasure/views/correction_view.py | 24 +- src/datasure/views/output_view_template.py | 1 + tests/checks/outliers/test_report_ui.py | 46 -- .../outliers/test_report_ui_corrections.py | 460 ++++++++++++++++++ tests/checks/outliers/test_review.py | 335 +++++++++++++ tests/processing/test_corrections.py | 85 ++++ tests/replication/test_package_builder.py | 2 +- tests/views/test_correction_view.py | 37 ++ 14 files changed, 1463 insertions(+), 120 deletions(-) create mode 100644 src/datasure/checks/outliers/review.py create mode 100644 tests/checks/outliers/test_report_ui_corrections.py create mode 100644 tests/checks/outliers/test_review.py diff --git a/CHANGELOG.md b/CHANGELOG.md index f6c8c19a..6ffccb12 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,6 +33,20 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 `apply_correction_entries`) renders the action, new-value and reason inputs for a prefilled KEY/column/current value, with namespaced widget keys. The Correct Data page now uses it — #296 +- **Outliers and constraints corrections**: Selecting a row in the constraint + violations or outlier inspection table opens the shared correction form, + prefilled with the row's KEY, column and current value, to modify the + value, remove it or accept it as valid (source and check type + `outliers`/`constraints`). Accepted flags are hidden and left out of the + metrics unless "Show reviewed" is on, and come back if the value changes or + the acceptance is removed. Accepting a hard violation needs a confirmation. + Flag review logic lives in the new Streamlit-free + `src/datasure/checks/outliers/review.py`; `outliers_report` takes the + dataset `alias`. Removed the unused `_render_outlier_table` — #298 +- **Correction log severity**: New `severity` column, `hard` on acceptances of + hard constraint violations (null otherwise and for legacy logs). + `CorrectionEntry.severity` sets it and is rejected on non-accept actions. + Hard acceptances are highlighted in the Correction Log — #298 ### Fixed diff --git a/docs/USER_GUIDE.md b/docs/USER_GUIDE.md index cbb2c6e0..abca97dd 100644 --- a/docs/USER_GUIDE.md +++ b/docs/USER_GUIDE.md @@ -469,6 +469,8 @@ Check type column shows which check it applies to. It stays in effect only while the value is unchanged. Accept entries are kept in `correction_log.csv` in the replication package but are not part of the corrections script. You can remove an accept entry with "Remove correction step" like any other entry. +Accepting a hard constraint violation sets Severity to `hard`, and those rows +are highlighted in red. To blank a cell, use "remove value": "modify value" needs a non-empty new value (`0` is valid). @@ -827,6 +829,25 @@ Visual analysis: - **Box Plot**: Distribution with outliers highlighted - **Table**: All records with outlier indicators +##### Correcting or Accepting Flagged Values + +Click a row in the constraint violations table or the outlier inspection table +to open a correction form below it. The KEY, column and current value are +filled in. Choose an action, enter a reason and click "Apply": + +- **modify value** or **remove value** corrects the data. The page reloads, and + the flag is updated or disappears. +- **accept** records that the flagged value is correct. The flag is hidden + and no longer counted in the metrics. Turn on "Show reviewed" to see + accepted flags with a Reviewed badge and the reason. + +Outlier and constraint acceptances are separate: accepting an outlier does not +accept a constraint violation on the same value. Accepting a **hard** +constraint violation needs an extra confirmation. An accepted flag comes back +if the value changes, or if you remove the acceptance on the Correct Data page. +Every entry appears in the Correction Log with source `outliers` or +`constraints`. + --- ### 6. Enumerator Stats Report diff --git a/src/datasure/checks/outliers/report_ui.py b/src/datasure/checks/outliers/report_ui.py index aa6023af..2a896803 100644 --- a/src/datasure/checks/outliers/report_ui.py +++ b/src/datasure/checks/outliers/report_ui.py @@ -1,6 +1,8 @@ """Report-rendering UI for the outliers report.""" from collections.abc import Callable +from dataclasses import dataclass, replace +from typing import Any import polars as pl import streamlit as st @@ -29,7 +31,28 @@ OutlierThresholds, SearchType, ) +from datasure.checks.outliers.review import ( + CONSTRAINTS, + OUTLIERS, + REVIEW_STATUS_COL, + FlagCheck, + FlagSelection, + allowed_actions, + clear_reviewed_flags, + mark_reviewed, + needs_hard_confirmation, + select_flag, + visible_flags, +) from datasure.checks.outliers.settings_ui import outliers_report_settings +from datasure.processing.correction_log import HARD_SEVERITY +from datasure.processing.corrections import CorrectionProcessor +from datasure.utils.correction_form import ( + apply_correction_entries, + get_current_value, + render_correction_inputs, + should_enable_apply_button, +) from datasure.utils.dataframe_utils import ColumnByType, sanitize_df_for_join from datasure.utils.duckdb_utils import duckdb_get_table, duckdb_save_table from datasure.utils.navigations_utils import demo_callout @@ -135,6 +158,193 @@ def _render_outlier_metrics( ) +# ============================================================================= +# Streamlit UI - Correcting and Accepting Flags +# ============================================================================= + +# Session-state key for the confirmation shown after the post-save rerun. +_SAVED_TOAST_KEY = "outliers_correction_toast" + + +@dataclass(frozen=True) +class ReviewContext: + """What the results tables need to correct or accept flagged values.""" + + processor: CorrectionProcessor + alias: str + + +def _mark_reviewed( + flags: pl.DataFrame, + settings: OutlierSettings, + check: FlagCheck, + review: ReviewContext | None, +) -> pl.DataFrame: + """Mark the flags accepted under `check`; a no-op if already marked.""" + if review is None or flags.is_empty() or REVIEW_STATUS_COL in flags.columns: + return flags + acceptances = review.processor.get_active_acceptances( + review.alias, check.check_type, settings.survey_key + ) + return mark_reviewed(flags, acceptances, settings.survey_key, check) + + +def _render_show_reviewed_toggle( + check: FlagCheck, review: ReviewContext | None +) -> bool: + """Render the "Show reviewed" toggle for a results table.""" + if review is None: + return False + return st.toggle( + "Show reviewed", + key=f"{check.check_type}_show_reviewed", + help="Show flags accepted as valid, with the reason they were accepted.", + ) + + +def _table_nonce_key(check_type: str) -> str: + return f"{check_type}_flags_table_nonce" + + +def _render_flags_table( + table: pl.DataFrame, + data: pl.DataFrame, + settings: OutlierSettings, + check: FlagCheck, + review: ReviewContext | None, + **dataframe_kwargs: Any, +) -> None: + """Render a results table; with `review`, a selected row opens the form.""" + if review is None: + st.dataframe(table, **dataframe_kwargs) + return + + # The nonce changes after each save, which drops the old selection. + nonce = st.session_state.get(_table_nonce_key(check.check_type), 0) + event = st.dataframe( + table, + key=f"{check.check_type}_flags_table_{nonce}", + on_select="rerun", + selection_mode="single-row", + **dataframe_kwargs, + ) + selection = select_flag( + table, list(event.selection.rows), settings.survey_key, check + ) + if selection is None: + st.caption("Select a row to correct the value or accept it as valid.") + return + + _render_flag_correction_form(data, settings, selection, review) + + +def _render_flag_correction_form( + data: pl.DataFrame, + settings: OutlierSettings, + selection: FlagSelection, + review: ReviewContext, +) -> None: + """Render the shared correction form for the selected flag. + + The form is prefilled with the flag's KEY and column and the value in + the data. Accepting a hard constraint violation needs an extra + confirmation and is logged with severity "hard". A successful save + reruns the page so the tables and metrics reflect it. + """ + key_col = settings.survey_key + key_value = selection.key_value + current_value = get_current_value(data, key_col, key_value, selection.column) + survey_id_value = ( + get_current_value(data, key_col, key_value, settings.survey_id) + if settings.survey_id + else None + ) + namespace = f"{selection.check_type}_{key_value}_{selection.column}" + + with st.container(border=True): + st.markdown(f"**{selection.column}** for KEY **{key_value}**") + if selection.reviewed: + st.info( + "This flag was accepted as valid. Remove the acceptance on the " + "Correct Data page to flag it again." + ) + + state = render_correction_inputs( + data, + key_col, + str(key_value), + key_namespace=namespace, + actions=allowed_actions(selection), + column=selection.column, + current_value=current_value, + check_type=selection.check_type, + survey_id_value=survey_id_value, + ) + + hard_accept = needs_hard_confirmation(selection, state.action) + confirmed = True + if hard_accept: + st.warning( + "This value breaks a hard constraint, a bound meant to be " + "absolute. The acceptance is highlighted in the Correction Log." + ) + confirmed = st.checkbox( + "I confirm this value is correct despite the hard constraint", + key=f"correction_hard_confirm_{namespace}", + ) + + apply_enabled = ( + should_enable_apply_button(state.action, state.reason, state.new_value) + and not state.validation_error + and confirmed + ) + if not st.button( + label="Apply", + key=f"correction_apply_{namespace}", + width="stretch", + disabled=not apply_enabled, + type="primary", + ): + return + + entry = state.to_entry() + if hard_accept: + entry = replace(entry, severity=HARD_SEVERITY) + if not apply_correction_entries( + review.processor, + review.alias, + key_col, + [entry], + source=selection.check_type, + ): + return + + nonce_key = _table_nonce_key(selection.check_type) + st.session_state[nonce_key] = st.session_state.get(nonce_key, 0) + 1 + st.session_state[_SAVED_TOAST_KEY] = ( + f"Saved {state.action} on {selection.column} for KEY {key_value}. " + "It is listed in the Correction Log on the Correct Data page." + ) + st.rerun() + + +def _show_saved_toast() -> None: + """Show the confirmation queued by a save before the page reran.""" + message = st.session_state.pop(_SAVED_TOAST_KEY, None) + if not message: + return + st.toast(message, icon=":material/check_circle:") + # A markdown link in the toast would open a new browser session and lose + # the selected project; a page link navigates within this session. + corrections_page = st.session_state.get("st_corr_page") + if corrections_page is not None: + st.page_link( + corrections_page, + label="Open the Correction Log", + icon=":material/cleaning_services:", + ) + + # ============================================================================= # Streamlit UI - Table Display # ============================================================================= @@ -149,9 +359,9 @@ def _render_display_columns_expander( ) -> list[str]: """Render the "Show more columns" expander and return the selected columns. - Shared by ``_render_constraint_violations_table`` and ``_render_outlier_table``, - which both let users add extra context columns to a results table, persisting - the selection to the settings file under ``settings_key``. + Used by ``_render_constraint_violations_table`` to let users add extra + context columns to the results table, persisting the selection to the + settings file under ``settings_key``. Parameters ---------- @@ -196,6 +406,7 @@ def _render_constraint_violations_table( violation_data: pl.DataFrame, settings: OutlierSettings, setting_file: str, + review: ReviewContext | None = None, ) -> None: """Render constraint violations table using Streamlit. @@ -209,11 +420,20 @@ def _render_constraint_violations_table( Outlier settings configuration. setting_file : str Path to settings file. + review : ReviewContext | None + If given, accepted violations are hidden (unless "Show reviewed" is + on) and selecting a row opens the correction form. """ if violation_data.is_empty(): st.info("No constraint violations detected.") return + show_reviewed = _render_show_reviewed_toggle(CONSTRAINTS, review) + violation_data = visible_flags( + _mark_reviewed(violation_data, settings, CONSTRAINTS, review), + show_reviewed=show_reviewed, + ) + all_columns = data.columns include_cols = _build_include_cols( @@ -272,68 +492,7 @@ def _render_constraint_violations_table( violation_type_expr.alias("violation type") ) - st.dataframe(violations_df) - - -def _render_outlier_table( - data: pl.DataFrame, - outliers_data: pl.DataFrame, - settings: OutlierSettings, - setting_file: str, -) -> None: - """Render outlier data table using Streamlit. - - Parameters - ---------- - data : pl.DataFrame - Original survey data. - outliers_data : pl.DataFrame - DataFrame containing outlier data. - settings : OutlierSettings - Outlier settings configuration. - setting_file : str - Path to settings file. - """ - if outliers_data.is_empty(): - st.info("No outliers detected in the selected columns.") - return - - all_columns = data.columns - - include_cols = _build_include_cols( - survey_key=settings.survey_key, - survey_id=settings.survey_id, - survey_date=settings.survey_date, - enumerator=settings.enumerator, - team=settings.team, - ) - - display_options = [col for col in all_columns if col not in include_cols] - - outlier_display_cols = _render_display_columns_expander( - setting_file, - "outlier_display_cols", - "outlier_display_cols", - display_options, - "Select additional columns to include in the outlier report.", - ) - - if outlier_display_cols: - include_cols.extend(outlier_display_cols) - - # select columns to display from data - display_df = data.select(include_cols) - outliers_df = sanitize_df_for_join(display_df, outliers_data, settings.survey_key) - display_df = display_df.join( - outliers_df, - on=settings.survey_key, - how="inner", - ) - - # show only rows with outliers - outlier_show_df = display_df.filter(pl.col("outlier reason") != "no outlier") - - st.dataframe(outlier_show_df) + _render_flags_table(violations_df, data, settings, CONSTRAINTS, review) def _render_outlier_column_inspection( @@ -341,6 +500,7 @@ def _render_outlier_column_inspection( outliers_data: pl.DataFrame, settings: OutlierSettings, setting_file: str, + review: ReviewContext | None = None, ) -> None: """Inspect outlier columns in the DataFrame. @@ -354,6 +514,9 @@ def _render_outlier_column_inspection( Outlier settings configuration. setting_file : str Path to settings file. + review : ReviewContext | None + If given, accepted outliers are hidden (unless "Show reviewed" is + on) and selecting a row opens the correction form. """ if outliers_data.is_empty(): st.info( @@ -447,6 +610,12 @@ def _render_outlier_column_inspection( if inspect_display_cols: include_cols.extend(inspect_display_cols) + show_reviewed = _render_show_reviewed_toggle(OUTLIERS, review) + outliers_data = visible_flags( + _mark_reviewed(outliers_data, settings, OUTLIERS, review), + show_reviewed=show_reviewed, + ) + # select columns to display from data display_df = data.select(include_cols) outliers_df = sanitize_df_for_join(display_df, outliers_data, settings.survey_key) @@ -456,8 +625,12 @@ def _render_outlier_column_inspection( how="inner", ) - st.dataframe( + _render_flags_table( display_df, + data, + settings, + OUTLIERS, + review, width="stretch", hide_index=False, ) @@ -1200,6 +1373,7 @@ def outliers_report( setting_file: str, config: dict, survey_columns: ColumnByType, + alias: str | None = None, ) -> None: """Create a comprehensive outliers report. @@ -1215,13 +1389,22 @@ def outliers_report( Path to settings file. config : dict Configuration dictionary. + survey_columns : ColumnByType + Columns of `data` by type. + alias : str | None + The survey dataset alias. If given, flagged values can be corrected + or accepted from the results tables, and accepted flags are hidden + and left out of the metrics. """ + review = ReviewContext(CorrectionProcessor(project_id), alias) if alias else None + # get column info categorical_columns = survey_columns.categorical_columns datetime_columns = survey_columns.datetime_columns numeric_columns = survey_columns.numeric_columns st.title("Outliers and Constraints Report") + _show_saved_toast() if is_demo_project(): demo_callout( @@ -1314,8 +1497,13 @@ def outliers_report( st.info("No constraint violations detected.") else: - # show constraint metrics - _render_constraint_metrics(constraint_violations) + constraint_violations = _mark_reviewed( + constraint_violations, outliers_settings, CONSTRAINTS, review + ) + # show constraint metrics, leaving out accepted violations + _render_constraint_metrics( + clear_reviewed_flags(constraint_violations, CONSTRAINTS) + ) # show constraint violations table st.subheader("Constraint Violations Details") @@ -1324,6 +1512,7 @@ def outliers_report( constraint_violations, outliers_settings, setting_file, + review=review, ) # show outliers metrics @@ -1355,8 +1544,11 @@ def outliers_report( st.info("No outliers detected.") else: - # show outlier metrics - _render_outlier_metrics(outlier_data, outliers_settings) + outlier_data = _mark_reviewed(outlier_data, outliers_settings, OUTLIERS, review) + # show outlier metrics, leaving out accepted outliers + _render_outlier_metrics( + clear_reviewed_flags(outlier_data, OUTLIERS), outliers_settings + ) # show outlier column inspection st.subheader("Inspect Columns") @@ -1366,6 +1558,7 @@ def outliers_report( outlier_data, outliers_settings, setting_file, + review=review, ) demo_callout( diff --git a/src/datasure/checks/outliers/review.py b/src/datasure/checks/outliers/review.py new file mode 100644 index 00000000..dc2e4905 --- /dev/null +++ b/src/datasure/checks/outliers/review.py @@ -0,0 +1,204 @@ +"""Review of outlier and constraint flags against accepted values. + +A flag is reviewed when the correction log holds an active acceptance for +its KEY and column under the same check type (see +`CorrectionProcessor.get_active_acceptances`). Reviewed flags are hidden from +the tables unless the user asks to see them, and are left out of the flag +counts. Outlier and constraint acceptances are independent: accepting an +outlier does not review a constraint violation on the same cell. + +Kept free of Streamlit so the logic can be tested without a running app. +""" + +from dataclasses import dataclass +from typing import Any + +import polars as pl + +from datasure.processing.correction_log import Action + +REVIEW_STATUS_COL = "review status" +REVIEW_REASON_COL = "review reason" +REVIEWED_BADGE = "Reviewed" + + +@dataclass(frozen=True) +class FlagCheck: + """How one check reports its flags in the computed results.""" + + check_type: str + reason_col: str + no_flag: str + + +OUTLIERS = FlagCheck("outliers", "outlier reason", "no outlier") +CONSTRAINTS = FlagCheck("constraints", "violation reason", "no violation") + +# Violation types (see `_render_constraint_violations_table`) of hard bounds. +_HARD_VIOLATION_TYPES = ("Hard Min", "Hard Max") + + +@dataclass(frozen=True) +class FlagSelection: + """The flag behind a selected table row, used to prefill the form.""" + + key_value: Any + column: str + check_type: str + flagged: bool + reviewed: bool + hard: bool + + +def _is_flagged(check: FlagCheck) -> pl.Expr: + return pl.col(check.reason_col).is_not_null() & ( + pl.col(check.reason_col) != check.no_flag + ) + + +def mark_reviewed( + flags: pl.DataFrame, + acceptances: pl.DataFrame, + survey_key: str, + check: FlagCheck, +) -> pl.DataFrame: + """Add review status and reason columns to computed flags. + + Parameters + ---------- + flags : pl.DataFrame + Output of `compute_outlier_output` or `compute_constraint_violations`: + one row per KEY and column name. + acceptances : pl.DataFrame + The active acceptances for `check`, as returned by + `CorrectionProcessor.get_active_acceptances`. + survey_key : str + The Survey KEY column in `flags`. + check : FlagCheck + The check that produced `flags`. + + Returns + ------- + pl.DataFrame + `flags` in the same order, plus `REVIEW_STATUS_COL` (the reviewed + badge, or null) and `REVIEW_REASON_COL` (the acceptance reason, or + null). Only flagged rows can be reviewed. + """ + if flags.is_empty(): + return flags + + # The log stores KEY as text; the latest acceptance's reason wins. + accepted = acceptances.select( + pl.col("KEY").cast(pl.String).alias("_review_key"), + pl.col("column").cast(pl.String).alias("_review_column"), + pl.lit(REVIEWED_BADGE).alias(REVIEW_STATUS_COL), + pl.col("reason").cast(pl.String).alias(REVIEW_REASON_COL), + ).unique(subset=["_review_key", "_review_column"], keep="last") + + marked = ( + flags.with_columns( + pl.col(survey_key).cast(pl.String).alias("_review_key"), + pl.col("column name").cast(pl.String).alias("_review_column"), + ) + .join( + accepted, + on=["_review_key", "_review_column"], + how="left", + maintain_order="left", + ) + .drop("_review_key", "_review_column") + ) + + flagged = _is_flagged(check) + return marked.with_columns( + pl.when(flagged).then(pl.col(REVIEW_STATUS_COL)).alias(REVIEW_STATUS_COL), + pl.when(flagged).then(pl.col(REVIEW_REASON_COL)).alias(REVIEW_REASON_COL), + ) + + +def _is_reviewed() -> pl.Expr: + return pl.col(REVIEW_STATUS_COL).is_not_null() + + +def clear_reviewed_flags(flags: pl.DataFrame, check: FlagCheck) -> pl.DataFrame: + """Return `flags` with reviewed flags reported as unflagged, for metrics. + + Rows are kept, so the number of columns checked is unchanged. + """ + if REVIEW_STATUS_COL not in flags.columns: + return flags + return flags.with_columns( + pl.when(_is_reviewed()) + .then(pl.lit(check.no_flag)) + .otherwise(pl.col(check.reason_col)) + .alias(check.reason_col) + ) + + +def visible_flags(flags: pl.DataFrame, *, show_reviewed: bool) -> pl.DataFrame: + """Return the rows and columns of `flags` to show in a results table. + + Reviewed flags and the review columns are hidden unless `show_reviewed`. + """ + if REVIEW_STATUS_COL not in flags.columns: + return flags + if show_reviewed: + return flags + return flags.filter(~_is_reviewed()).drop(REVIEW_STATUS_COL, REVIEW_REASON_COL) + + +def select_flag( + table: pl.DataFrame, + rows: list[int], + survey_key: str, + check: FlagCheck, +) -> FlagSelection | None: + """Return the flag behind the selected row of a results table. + + Parameters + ---------- + table : pl.DataFrame + The table as displayed. + rows : list[int] + Selected row positions, as reported by `st.dataframe` selection. + survey_key : str + The Survey KEY column in `table`. + check : FlagCheck + The check the table shows. + + Returns + ------- + FlagSelection | None + The selection, or None if no row is selected or the position no + longer exists in `table`. + """ + if not rows or not 0 <= rows[0] < table.height: + return None + + row = table.row(rows[0], named=True) + reason = row.get(check.reason_col) + return FlagSelection( + key_value=row[survey_key], + column=row["column name"], + check_type=check.check_type, + flagged=reason is not None and reason != check.no_flag, + reviewed=row.get(REVIEW_STATUS_COL) is not None, + hard=row.get("violation type") in _HARD_VIOLATION_TYPES, + ) + + +def allowed_actions(selection: FlagSelection) -> list[Action]: + """Return the actions the correction form offers for `selection`. + + A value can be accepted only while it is flagged and not yet reviewed. + Rows are removed from the Corrections page, not from a check page. + """ + actions = [Action.MODIFY_VALUE, Action.REMOVE_VALUE] + if selection.flagged and not selection.reviewed: + actions.append(Action.ACCEPT) + return actions + + +def needs_hard_confirmation(selection: FlagSelection, action: Action) -> bool: + """Whether applying `action` needs the hard-violation confirmation step.""" + return action == Action.ACCEPT and selection.hard diff --git a/src/datasure/processing/correction_log.py b/src/datasure/processing/correction_log.py index 81b54f25..dd42f56c 100644 --- a/src/datasure/processing/correction_log.py +++ b/src/datasure/processing/correction_log.py @@ -35,6 +35,9 @@ class Action(StrEnum): ACCEPT_CHECK_TYPES = ("outliers", "constraints", "backchecks", "duplicates", "gps") +# `severity` of an acceptance that overrides a hard constraint bound. +HARD_SEVERITY = "hard" + # Full schema of a persisted correction log (`corr_log_{alias}`), in column order. CORRECTION_LOG_SCHEMA: dict[str, pl.DataType] = { "date": pl.Datetime("us"), @@ -49,6 +52,9 @@ class Action(StrEnum): "status_reason": pl.String, "source": pl.String, "check_type": pl.String, + # For "accept", how serious the accepted flag is: "hard" for a hard + # constraint violation, null otherwise. + "severity": pl.String, } # Values given to columns that were added to the log after some logs were @@ -59,6 +65,7 @@ class Action(StrEnum): "status_reason": None, "source": CORRECTIONS_PAGE_SOURCE, "check_type": None, + "severity": None, } diff --git a/src/datasure/processing/corrections.py b/src/datasure/processing/corrections.py index e50e0bf2..f5f5f97e 100644 --- a/src/datasure/processing/corrections.py +++ b/src/datasure/processing/corrections.py @@ -191,6 +191,7 @@ def _build_log_row( reason: str, source: str, check_type: str | None, + severity: str | None = None, ) -> dict[str, Any]: """Build one correction-log row. @@ -210,6 +211,7 @@ def _build_log_row( "status_reason": None, "source": str(source), "check_type": check_type, + "severity": severity, } @@ -237,6 +239,9 @@ class CorrectionEntry: The Survey ID value for this KEY, recorded in the log's ID column check_type : str | None For "accept", the check whose flag is accepted + severity : str | None + For "accept", how serious the accepted flag is: "hard" for a hard + constraint violation, otherwise None """ key_value: str @@ -247,6 +252,7 @@ class CorrectionEntry: new_value: Any = None survey_id_value: Any = None check_type: str | None = None + severity: str | None = None # The cached methods below hash `self` by its project so that two projects @@ -663,6 +669,7 @@ def apply_corrections( reason=entry.reason, source=source, check_type=entry.check_type, + severity=entry.severity, ) for entry in entries ] @@ -698,6 +705,12 @@ def _apply_entry( ) return data + if entry.severity is not None: + raise ValueError( + f"Only acceptances record a severity, not {entry.action} " + f"on {entry.key_value}" + ) + if entry.action not in CORRECTION_ACTIONS: raise ValueError(f"Unknown correction action '{entry.action}'") @@ -1252,6 +1265,7 @@ def get_correction_summary(self, alias: str) -> list[dict[str, Any]]: "action_index": f"{index} - {action} - {description}", "action": action, "check_type": row["check_type"], + "severity": row["severity"], "description": description, "key_value": key_value, "column": column, diff --git a/src/datasure/views/correction_view.py b/src/datasure/views/correction_view.py index d9c18b5f..7b6a3633 100644 --- a/src/datasure/views/correction_view.py +++ b/src/datasure/views/correction_view.py @@ -14,6 +14,7 @@ from datasure.processing.correction_log import ( CORRECTIONS_PAGE_SOURCE, + HARD_SEVERITY, Action, ensure_log_columns, ) @@ -536,8 +537,8 @@ def _build_correction_log_display(correction_log: pl.DataFrame) -> pl.DataFrame: Backfills columns missing from logs saved before they existed, orders columns so status/status_reason sit right after action, and relabels the "ID" column as "Survey ID" for display. "accept" rows carry the check - whose flag was accepted in check_type; source names the page that made - each entry. + whose flag was accepted in check_type and, for a hard constraint + violation, severity "hard"; source names the page that made each entry. Parameters ---------- @@ -560,6 +561,7 @@ def _build_correction_log_display(correction_log: pl.DataFrame) -> pl.DataFrame: "status", "status_reason", "check_type", + "severity", "column", "current_value", "new_value", @@ -571,6 +573,20 @@ def _build_correction_log_display(correction_log: pl.DataFrame) -> pl.DataFrame: return correction_log.select(display_columns).rename({"ID": "Survey ID"}) +def highlight_hard_acceptance(row: Any) -> list[str]: + """Style every cell of a hard-violation acceptance in the Correction Log. + + Used with a pandas ``Styler`` (``df.style.apply(highlight_hard_acceptance, + axis=1)``). Accepting a value that breaks a hard constraint overrides a + bound meant to be absolute, so those rows stand out for review. + """ + is_hard_accept = row.get("action") == Action.ACCEPT and ( + row.get("severity") == HARD_SEVERITY + ) + style = "background-color: rgba(220, 53, 69, 0.15)" if is_hard_accept else "" + return [style] * len(row) + + @st.fragment def render_correction_log( correction_processor: CorrectionProcessor, alias: str, tab_index: int @@ -600,7 +616,9 @@ def render_correction_log( log_display = _build_correction_log_display(correction_log).to_pandas() st.dataframe( - log_display.style.map(highlight_status, subset=["status"]), + log_display.style.apply(highlight_hard_acceptance, axis=1).map( + highlight_status, subset=["status"] + ), width="stretch", ) diff --git a/src/datasure/views/output_view_template.py b/src/datasure/views/output_view_template.py index 8f041223..1e628b36 100644 --- a/src/datasure/views/output_view_template.py +++ b/src/datasure/views/output_view_template.py @@ -369,6 +369,7 @@ def render_check_tabs(project_id: str, config: PageConfig, data: CheckData) -> N config.setting_file, outliers_config, survey_columns, + alias=config.survey_data_name, ) with gps_checks: diff --git a/tests/checks/outliers/test_report_ui.py b/tests/checks/outliers/test_report_ui.py index 71353b2c..11571b26 100644 --- a/tests/checks/outliers/test_report_ui.py +++ b/tests/checks/outliers/test_report_ui.py @@ -30,7 +30,6 @@ _render_outlier_metrics, _render_outlier_options, _render_outlier_settings_table, - _render_outlier_table, _render_search_type_selection, _update_outlier_column_config, _validate_constraint_settings, @@ -471,51 +470,6 @@ def test_non_empty_with_extra_display_cols( st_mock.dataframe.assert_called_once() -# ============================================================================ -# TESTS: _render_outlier_table -# ============================================================================ - - -class TestRenderOutlierTable: - """Test _render_outlier_table function.""" - - def test_empty_data_shows_info(self, base_survey_data, outlier_settings): - with patch("datasure.checks.outliers.report_ui.st") as st_mock: - _render_outlier_table( - base_survey_data, - pl.DataFrame(), - outlier_settings, - "settings.json", - ) - st_mock.info.assert_called_once() - - def test_non_empty_data_shows_dataframe(self, base_survey_data, outlier_settings): - outliers_data = pl.DataFrame( - { - "survey_key": ["K001"], - "column name": ["col1"], - "outlier reason": ["Value is above upper bound 50.00"], - } - ) - with ( - patch("datasure.checks.outliers.report_ui.st") as st_mock, - patch( - "datasure.checks.outliers.report_ui.load_check_settings", - return_value={}, - ), - patch("datasure.checks.outliers.report_ui.save_check_settings"), - ): - st_mock.columns.side_effect = _columns_side_effect - st_mock.multiselect.return_value = [] - _render_outlier_table( - base_survey_data, - outliers_data, - outlier_settings, - "settings.json", - ) - st_mock.dataframe.assert_called_once() - - # ============================================================================ # TESTS: _render_outlier_column_inspection # ============================================================================ diff --git a/tests/checks/outliers/test_report_ui_corrections.py b/tests/checks/outliers/test_report_ui_corrections.py new file mode 100644 index 00000000..bcbad4cd --- /dev/null +++ b/tests/checks/outliers/test_report_ui_corrections.py @@ -0,0 +1,460 @@ +"""Tests for correcting and accepting flags from the outliers report tables.""" + +from unittest.mock import MagicMock, patch + +import polars as pl +import pytest + +from datasure.checks.outliers.models import OutlierSettings +from datasure.checks.outliers.report_ui import ( + ReviewContext, + _render_constraint_violations_table, + _render_flag_correction_form, + _render_outlier_column_inspection, + outliers_report, +) +from datasure.checks.outliers.review import FlagSelection +from datasure.processing.correction_log import Action +from datasure.utils.correction_form import CorrectionFormState +from datasure.utils.dataframe_utils import ColumnByType +from tests.checks.outliers.conftest import _columns_side_effect + +MODULE = "datasure.checks.outliers.report_ui" + + +@pytest.fixture +def settings() -> OutlierSettings: + return OutlierSettings(survey_key="KEY", survey_id="hhid") + + +@pytest.fixture +def data() -> pl.DataFrame: + return pl.DataFrame( + { + "KEY": ["K1", "K2", "K3"], + "hhid": ["H1", "H2", "H3"], + "age": [150, 70, 30], + } + ) + + +@pytest.fixture +def violations() -> pl.DataFrame: + """`compute_constraint_violations` output for `data`.""" + return pl.DataFrame( + { + "KEY": ["K1", "K2", "K3"], + "column name": ["age", "age", "age"], + "column value": [150.0, 70.0, 30.0], + "hard_min": [0.0] * 3, + "soft_min": [None] * 3, + "soft_max": [65.0] * 3, + "hard_max": [100.0] * 3, + "violation reason": [ + "Value is above hard maximum 100.0", + "Value is above soft maximum 65.0", + "no violation", + ], + } + ) + + +def _acceptances(*rows: tuple[str, str, str]) -> pl.DataFrame: + """Active acceptances with (KEY, column, reason) rows.""" + return pl.DataFrame( + { + "KEY": [r[0] for r in rows], + "action": ["accept"] * len(rows), + "column": [r[1] for r in rows], + "reason": [r[2] for r in rows], + }, + schema={ + "KEY": pl.String, + "action": pl.String, + "column": pl.String, + "reason": pl.String, + }, + ) + + +def _review(acceptances_by_check: dict[str, pl.DataFrame] | None = None): + acceptances_by_check = acceptances_by_check or {} + processor = MagicMock() + processor.get_active_acceptances.side_effect = lambda alias, check_type, key_col: ( + acceptances_by_check.get(check_type, _acceptances()) + ) + return ReviewContext(processor=processor, alias="survey") + + +def _st_mock(selected_rows: list[int] | None = None, show_reviewed=False): + st_mock = MagicMock() + st_mock.columns.side_effect = _columns_side_effect + st_mock.multiselect.return_value = [] + st_mock.toggle.return_value = show_reviewed + st_mock.session_state = {} + st_mock.dataframe.return_value.selection.rows = selected_rows or [] + return st_mock + + +def _shown_table(st_mock) -> pl.DataFrame: + return st_mock.dataframe.call_args.args[0] + + +def _selection(**overrides) -> FlagSelection: + values = { + "key_value": "K2", + "column": "age", + "check_type": "constraints", + "flagged": True, + "reviewed": False, + "hard": False, + } + return FlagSelection(**(values | overrides)) + + +def _form_state(action=Action.ACCEPT, reason="verified", **overrides): + values = { + "key_value": "K2", + "action": action, + "column": "age", + "current_value": 70, + "reason": reason, + "check_type": "constraints" if action == Action.ACCEPT else None, + "survey_id_value": "H2", + } + return CorrectionFormState(**(values | overrides)) + + +class TestConstraintTableSelection: + def _render(self, data, violations, settings, st_mock, review): + with ( + patch(f"{MODULE}.st", st_mock), + patch(f"{MODULE}.load_check_settings", return_value={}), + patch(f"{MODULE}.save_check_settings"), + patch(f"{MODULE}._render_flag_correction_form") as form, + ): + _render_constraint_violations_table( + data, violations, settings, "settings.json", review=review + ) + return form + + def test_table_allows_single_row_selection(self, data, violations, settings): + st_mock = _st_mock() + + self._render(data, violations, settings, st_mock, _review()) + + kwargs = st_mock.dataframe.call_args.kwargs + assert kwargs["on_select"] == "rerun" + assert kwargs["selection_mode"] == "single-row" + + def test_selecting_a_row_opens_the_form_prefilled_from_it( + self, data, violations, settings + ): + st_mock = _st_mock(selected_rows=[0]) + review = _review() + + form = self._render(data, violations, settings, st_mock, review) + + form.assert_called_once() + _, _, selection, passed_review = form.call_args.args + assert selection == FlagSelection( + key_value="K1", + column="age", + check_type="constraints", + flagged=True, + reviewed=False, + hard=True, + ) + assert passed_review is review + + def test_no_selection_opens_no_form(self, data, violations, settings): + form = self._render(data, violations, settings, _st_mock(), _review()) + + form.assert_not_called() + + def test_accepted_violations_are_hidden(self, data, violations, settings): + st_mock = _st_mock() + review = _review({"constraints": _acceptances(("K1", "age", "verified"))}) + + self._render(data, violations, settings, st_mock, review) + + assert _shown_table(st_mock)["KEY"].to_list() == ["K2"] + + def test_show_reviewed_shows_them_with_badge_and_reason( + self, data, violations, settings + ): + st_mock = _st_mock(show_reviewed=True) + review = _review({"constraints": _acceptances(("K1", "age", "verified"))}) + + self._render(data, violations, settings, st_mock, review) + + table = _shown_table(st_mock) + assert table.select("KEY", "review status", "review reason").rows() == [ + ("K1", "Reviewed", "verified"), + ("K2", None, None), + ] + + def test_outlier_acceptance_does_not_hide_a_constraint_violation( + self, data, violations, settings + ): + st_mock = _st_mock() + review = _review({"outliers": _acceptances(("K1", "age", "verified"))}) + + self._render(data, violations, settings, st_mock, review) + + assert _shown_table(st_mock)["KEY"].to_list() == ["K1", "K2"] + + def test_without_review_the_table_is_read_only(self, data, violations, settings): + st_mock = _st_mock() + + self._render(data, violations, settings, st_mock, None) + + assert "on_select" not in st_mock.dataframe.call_args.kwargs + + +class TestOutlierTableSelection: + @pytest.fixture + def outliers(self) -> pl.DataFrame: + return pl.DataFrame( + { + "KEY": ["K1", "K2", "K3"], + "column name": ["age"] * 3, + "column value": [150.0, 70.0, 30.0], + "outlier reason": [ + "Value is above upper bound 120.00", + "no outlier", + "no outlier", + ], + } + ) + + def _render(self, data, outliers, settings, st_mock, review): + with ( + patch(f"{MODULE}.st", st_mock), + patch(f"{MODULE}.load_check_settings", return_value={}), + patch(f"{MODULE}.save_check_settings"), + patch(f"{MODULE}._create_descriptive_stats", return_value=pl.DataFrame()), + patch(f"{MODULE}._create_box_plot"), + patch(f"{MODULE}._render_flag_correction_form") as form, + ): + st_mock.selectbox.return_value = "age" + _render_outlier_column_inspection( + data, outliers, settings, "settings.json", review=review + ) + return form + + def test_selecting_a_row_opens_the_form_for_outliers( + self, data, outliers, settings + ): + st_mock = _st_mock(selected_rows=[0]) + + form = self._render(data, outliers, settings, st_mock, _review()) + + selection = form.call_args.args[2] + assert selection.key_value == "K1" + assert selection.column == "age" + assert selection.check_type == "outliers" + assert selection.hard is False + + def test_accepted_outliers_are_hidden(self, data, outliers, settings): + st_mock = _st_mock() + review = _review({"outliers": _acceptances(("K1", "age", "verified"))}) + + self._render(data, outliers, settings, st_mock, review) + + assert "K1" not in _shown_table(st_mock)["KEY"].to_list() + + +class TestFlagCorrectionForm: + def _render(self, data, settings, selection, review, state, *, apply, confirm): + st_mock = _st_mock() + st_mock.button.return_value = apply + st_mock.checkbox.return_value = confirm + with ( + patch(f"{MODULE}.st", st_mock), + patch(f"{MODULE}.render_correction_inputs", return_value=state) as inputs, + patch( + f"{MODULE}.apply_correction_entries", return_value=True + ) as apply_entries, + ): + _render_flag_correction_form(data, settings, selection, review) + return st_mock, inputs, apply_entries + + def test_form_is_prefilled_from_the_selection_and_data(self, data, settings): + _, inputs, _ = self._render( + data, + settings, + _selection(), + _review(), + _form_state(), + apply=False, + confirm=False, + ) + + args, kwargs = inputs.call_args + assert args == (data, "KEY", "K2") + assert kwargs["column"] == "age" + assert kwargs["current_value"] == 70 + assert kwargs["survey_id_value"] == "H2" + assert kwargs["check_type"] == "constraints" + assert kwargs["actions"] == [ + Action.MODIFY_VALUE, + Action.REMOVE_VALUE, + Action.ACCEPT, + ] + + def test_apply_saves_with_the_check_as_source_and_reruns(self, data, settings): + review = _review() + + st_mock, _, apply_entries = self._render( + data, + settings, + _selection(), + review, + _form_state(action=Action.MODIFY_VALUE, new_value="60"), + apply=True, + confirm=False, + ) + + args, kwargs = apply_entries.call_args + assert args[:3] == (review.processor, "survey", "KEY") + (entry,) = args[3] + assert entry.action == Action.MODIFY_VALUE + assert entry.new_value == "60" + assert kwargs["source"] == "constraints" + st_mock.rerun.assert_called_once() + + def test_a_save_queues_a_toast_for_after_the_rerun(self, data, settings): + st_mock, _, _ = self._render( + data, + settings, + _selection(), + _review(), + _form_state(), + apply=True, + confirm=False, + ) + + assert any("toast" in key for key in st_mock.session_state) + + def test_hard_violation_accept_is_disabled_until_confirmed(self, data, settings): + st_mock, _, apply_entries = self._render( + data, + settings, + _selection(key_value="K1", hard=True), + _review(), + _form_state(key_value="K1", current_value=150), + apply=False, + confirm=False, + ) + + st_mock.checkbox.assert_called_once() + assert st_mock.button.call_args.kwargs["disabled"] is True + apply_entries.assert_not_called() + + def test_confirmed_hard_violation_accept_is_logged_as_hard(self, data, settings): + st_mock, _, apply_entries = self._render( + data, + settings, + _selection(key_value="K1", hard=True), + _review(), + _form_state(key_value="K1", current_value=150), + apply=True, + confirm=True, + ) + + assert st_mock.button.call_args.kwargs["disabled"] is False + (entry,) = apply_entries.call_args.args[3] + assert entry.action == Action.ACCEPT + assert entry.severity == "hard" + + def test_soft_violation_accept_needs_no_confirmation(self, data, settings): + st_mock, _, apply_entries = self._render( + data, + settings, + _selection(), + _review(), + _form_state(), + apply=True, + confirm=False, + ) + + st_mock.checkbox.assert_not_called() + (entry,) = apply_entries.call_args.args[3] + assert entry.severity is None + + def test_modifying_a_hard_violation_needs_no_confirmation(self, data, settings): + st_mock, _, _ = self._render( + data, + settings, + _selection(key_value="K1", hard=True), + _review(), + _form_state(action=Action.MODIFY_VALUE, new_value="90"), + apply=False, + confirm=False, + ) + + st_mock.checkbox.assert_not_called() + + def test_failed_save_does_not_rerun(self, data, settings): + st_mock = _st_mock() + st_mock.button.return_value = True + with ( + patch(f"{MODULE}.st", st_mock), + patch(f"{MODULE}.render_correction_inputs", return_value=_form_state()), + patch(f"{MODULE}.apply_correction_entries", return_value=False), + ): + _render_flag_correction_form(data, settings, _selection(), _review()) + + st_mock.rerun.assert_not_called() + + +class TestMetricsExcludeAcceptedFlags: + def test_metrics_do_not_count_accepted_flags(self, data, violations): + config = {"survey_key": "KEY", "survey_id": "hhid"} + columns = ColumnByType( + all_columns=data.columns, + categorical_columns=[], + datetime_columns=[], + numeric_columns=["age"], + string_columns=[], + integer_columns=["age"], + ) + review_processor = _review( + {"constraints": _acceptances(("K1", "age", "verified"))} + ).processor + st_mock = _st_mock() + with ( + patch(f"{MODULE}.st", st_mock), + patch( + f"{MODULE}.outliers_report_settings", + return_value=OutlierSettings(**config), + ), + patch(f"{MODULE}._render_outlier_column_actions"), + patch( + f"{MODULE}.duckdb_get_table", + return_value=pl.DataFrame({"column_name": [["age"]]}), + ), + patch(f"{MODULE}._update_unlocked_cols", side_effect=lambda df, _: df), + patch(f"{MODULE}.duckdb_save_table"), + patch(f"{MODULE}.compute_constraint_violations", return_value=violations), + patch(f"{MODULE}.compute_outlier_output", return_value=pl.DataFrame()), + patch(f"{MODULE}.CorrectionProcessor", return_value=review_processor), + patch(f"{MODULE}._render_constraint_metrics") as metrics, + patch(f"{MODULE}._render_constraint_violations_table") as table, + ): + outliers_report( + "proj1", + "page1", + data, + "settings.json", + config, + columns, + alias="survey", + ) + + counted = metrics.call_args.args[0] + assert counted.filter(pl.col("violation reason") != "no violation")[ + "KEY" + ].to_list() == ["K2"] + assert table.call_args.kwargs["review"].alias == "survey" diff --git a/tests/checks/outliers/test_review.py b/tests/checks/outliers/test_review.py new file mode 100644 index 00000000..372d746f --- /dev/null +++ b/tests/checks/outliers/test_review.py @@ -0,0 +1,335 @@ +"""Tests for datasure.checks.outliers.review.""" + +import polars as pl +import pytest + +from datasure.checks.outliers.review import ( + CONSTRAINTS, + OUTLIERS, + REVIEW_REASON_COL, + REVIEW_STATUS_COL, + REVIEWED_BADGE, + FlagSelection, + allowed_actions, + clear_reviewed_flags, + mark_reviewed, + needs_hard_confirmation, + select_flag, + visible_flags, +) +from datasure.processing.correction_log import Action, empty_correction_log + + +def _acceptances(rows: list[dict]) -> pl.DataFrame: + """Build accept rows shaped like `get_active_acceptances` output.""" + base = empty_correction_log() + if not rows: + return base + return pl.DataFrame( + [ + { + "KEY": r["KEY"], + "action": "accept", + "column": r["column"], + "current_value": r.get("current_value"), + "reason": r.get("reason", "checked"), + "check_type": r.get("check_type", "outliers"), + } + for r in rows + ] + ).select( + pl.col(c).cast(pl.String) + for c in ["KEY", "action", "column", "current_value", "reason", "check_type"] + ) + + +@pytest.fixture +def outlier_flags() -> pl.DataFrame: + return pl.DataFrame( + { + "survey_key": ["K1", "K2", "K3", "K1"], + "column name": ["age", "age", "age", "income"], + "column value": [99.0, 30.0, 120.0, 5000.0], + "outlier reason": [ + "Value is above upper bound 80.00", + "no outlier", + "Value is above upper bound 80.00", + "Value is above upper bound 900.00", + ], + } + ) + + +class TestMarkReviewed: + def test_flags_matching_an_acceptance_are_reviewed_with_its_reason( + self, outlier_flags + ): + acceptances = _acceptances( + [{"KEY": "K1", "column": "age", "reason": "verified by phone"}] + ) + + result = mark_reviewed(outlier_flags, acceptances, "survey_key", OUTLIERS) + + assert result[REVIEW_STATUS_COL].to_list() == [ + REVIEWED_BADGE, + None, + None, + None, + ] + assert result[REVIEW_REASON_COL].to_list() == [ + "verified by phone", + None, + None, + None, + ] + + def test_acceptance_on_another_column_of_the_same_key_does_not_match( + self, outlier_flags + ): + acceptances = _acceptances([{"KEY": "K1", "column": "income"}]) + + result = mark_reviewed(outlier_flags, acceptances, "survey_key", OUTLIERS) + + assert result[REVIEW_STATUS_COL].to_list() == [ + None, + None, + None, + REVIEWED_BADGE, + ] + + def test_unflagged_rows_are_never_reviewed(self, outlier_flags): + acceptances = _acceptances([{"KEY": "K2", "column": "age"}]) + + result = mark_reviewed(outlier_flags, acceptances, "survey_key", OUTLIERS) + + assert result[REVIEW_STATUS_COL].null_count() == result.height + + def test_keeps_row_order_and_columns(self, outlier_flags): + acceptances = _acceptances([{"KEY": "K3", "column": "age"}]) + + result = mark_reviewed(outlier_flags, acceptances, "survey_key", OUTLIERS) + + assert result.columns == [ + *outlier_flags.columns, + REVIEW_STATUS_COL, + REVIEW_REASON_COL, + ] + assert result.drop(REVIEW_STATUS_COL, REVIEW_REASON_COL).equals(outlier_flags) + + def test_matches_non_string_keys_as_text(self): + flags = pl.DataFrame( + { + "survey_key": [1, 2], + "column name": ["age", "age"], + "violation reason": ["Value is above hard maximum 100", "no violation"], + } + ) + acceptances = _acceptances( + [{"KEY": "1", "column": "age", "check_type": "constraints"}] + ) + + result = mark_reviewed(flags, acceptances, "survey_key", CONSTRAINTS) + + assert result[REVIEW_STATUS_COL].to_list() == [REVIEWED_BADGE, None] + + def test_repeated_acceptances_use_the_latest_reason(self, outlier_flags): + acceptances = _acceptances( + [ + {"KEY": "K1", "column": "age", "reason": "first"}, + {"KEY": "K1", "column": "age", "reason": "second"}, + ] + ) + + result = mark_reviewed(outlier_flags, acceptances, "survey_key", OUTLIERS) + + assert result.height == outlier_flags.height + assert result[REVIEW_REASON_COL][0] == "second" + + def test_no_acceptances_marks_nothing(self, outlier_flags): + result = mark_reviewed( + outlier_flags, empty_correction_log(), "survey_key", OUTLIERS + ) + + assert result[REVIEW_STATUS_COL].null_count() == result.height + + def test_empty_flags_are_returned_unchanged(self): + result = mark_reviewed(pl.DataFrame(), _acceptances([]), "survey_key", OUTLIERS) + + assert result.is_empty() + + +class TestClearReviewedFlags: + def test_reviewed_flags_no_longer_count_as_flags(self, outlier_flags): + marked = mark_reviewed( + outlier_flags, + _acceptances([{"KEY": "K1", "column": "age"}]), + "survey_key", + OUTLIERS, + ) + + result = clear_reviewed_flags(marked, OUTLIERS) + + assert result["outlier reason"].to_list() == [ + "no outlier", + "no outlier", + "Value is above upper bound 80.00", + "Value is above upper bound 900.00", + ] + + def test_keeps_rows_so_checked_columns_are_still_counted(self, outlier_flags): + acceptances = _acceptances( + [{"KEY": "K1", "column": "income"}, {"KEY": "K1", "column": "age"}] + ) + marked = mark_reviewed(outlier_flags, acceptances, "survey_key", OUTLIERS) + + result = clear_reviewed_flags(marked, OUTLIERS) + + assert result.height == outlier_flags.height + assert set(result["column name"]) == {"age", "income"} + + def test_data_without_review_columns_is_unchanged(self, outlier_flags): + assert clear_reviewed_flags(outlier_flags, OUTLIERS).equals(outlier_flags) + + +class TestVisibleFlags: + @pytest.fixture + def marked(self, outlier_flags): + return mark_reviewed( + outlier_flags, + _acceptances([{"KEY": "K1", "column": "age", "reason": "ok"}]), + "survey_key", + OUTLIERS, + ) + + def test_hides_reviewed_flags_and_review_columns(self, marked): + result = visible_flags(marked, show_reviewed=False) + + assert result.height == 3 + assert REVIEW_STATUS_COL not in result.columns + assert REVIEW_REASON_COL not in result.columns + + def test_show_reviewed_keeps_them_with_badge_and_reason(self, marked): + result = visible_flags(marked, show_reviewed=True) + + assert result.height == 4 + assert result.row(0, named=True)[REVIEW_STATUS_COL] == REVIEWED_BADGE + assert result.row(0, named=True)[REVIEW_REASON_COL] == "ok" + + def test_data_without_review_columns_is_unchanged(self, outlier_flags): + assert visible_flags(outlier_flags, show_reviewed=False).equals(outlier_flags) + + +@pytest.fixture +def constraint_table() -> pl.DataFrame: + """A constraint table as displayed: flagged rows plus the violation type.""" + return pl.DataFrame( + { + "survey_key": ["K1", "K2"], + "survey_id": ["S1", "S2"], + "column name": ["age", "age"], + "column value": [150.0, 70.0], + "violation reason": [ + "Value is above hard maximum 100", + "Value is above soft maximum 65", + ], + "violation type": ["Hard Max", "Soft Max"], + } + ) + + +class TestSelectFlag: + def test_prefills_key_and_column_from_the_selected_row(self, constraint_table): + selection = select_flag(constraint_table, [1], "survey_key", CONSTRAINTS) + + assert selection == FlagSelection( + key_value="K2", + column="age", + check_type="constraints", + flagged=True, + reviewed=False, + hard=False, + ) + + def test_hard_violations_are_marked_hard(self, constraint_table): + selection = select_flag(constraint_table, [0], "survey_key", CONSTRAINTS) + + assert selection.hard is True + + def test_outliers_are_never_hard(self, outlier_flags): + selection = select_flag(outlier_flags, [0], "survey_key", OUTLIERS) + + assert selection.check_type == "outliers" + assert selection.flagged is True + assert selection.hard is False + + def test_unflagged_rows_are_reported_as_unflagged(self, outlier_flags): + selection = select_flag(outlier_flags, [1], "survey_key", OUTLIERS) + + assert selection.flagged is False + + def test_reviewed_rows_are_reported_as_reviewed(self, outlier_flags): + table = visible_flags( + mark_reviewed( + outlier_flags, + _acceptances([{"KEY": "K1", "column": "age"}]), + "survey_key", + OUTLIERS, + ), + show_reviewed=True, + ) + + selection = select_flag(table, [0], "survey_key", OUTLIERS) + + assert selection.reviewed is True + + def test_keeps_the_key_value_as_stored_in_the_data(self): + table = pl.DataFrame( + { + "survey_key": [7], + "column name": ["age"], + "outlier reason": ["Value is above upper bound 80.00"], + } + ) + + assert select_flag(table, [0], "survey_key", OUTLIERS).key_value == 7 + + @pytest.mark.parametrize("rows", [[], [5], [-1]]) + def test_no_or_stale_selection_returns_none(self, constraint_table, rows): + assert select_flag(constraint_table, rows, "survey_key", CONSTRAINTS) is None + + +class TestAllowedActions: + def _selection(self, **overrides) -> FlagSelection: + values = { + "key_value": "K1", + "column": "age", + "check_type": "outliers", + "flagged": True, + "reviewed": False, + "hard": False, + } + return FlagSelection(**(values | overrides)) + + def test_flagged_value_can_be_modified_removed_or_accepted(self): + assert allowed_actions(self._selection()) == [ + Action.MODIFY_VALUE, + Action.REMOVE_VALUE, + Action.ACCEPT, + ] + + def test_unflagged_value_cannot_be_accepted(self): + assert Action.ACCEPT not in allowed_actions(self._selection(flagged=False)) + + def test_reviewed_value_cannot_be_accepted_again(self): + assert Action.ACCEPT not in allowed_actions(self._selection(reviewed=True)) + + def test_rows_are_never_removed_from_a_check_page(self): + assert Action.REMOVE_ROW not in allowed_actions(self._selection()) + + def test_only_accepting_a_hard_violation_needs_confirmation(self): + hard = self._selection(check_type="constraints", hard=True) + soft = self._selection(check_type="constraints", hard=False) + + assert needs_hard_confirmation(hard, Action.ACCEPT) is True + assert needs_hard_confirmation(hard, Action.MODIFY_VALUE) is False + assert needs_hard_confirmation(soft, Action.ACCEPT) is False diff --git a/tests/processing/test_corrections.py b/tests/processing/test_corrections.py index fba74060..b0e4f6ef 100644 --- a/tests/processing/test_corrections.py +++ b/tests/processing/test_corrections.py @@ -1169,6 +1169,7 @@ def test_removing_the_only_entry_leaves_an_empty_log_with_full_schema( "status_reason", "source", "check_type", + "severity", ] @@ -1829,3 +1830,87 @@ def test_reports_corrections_that_no_longer_apply(self, store, sample_data): assert len(failures) == 1 assert processor.get_correction_log("survey")["status"].to_list() == ["Failed"] + + +class TestAcceptanceSeverity: + """Hard constraint acceptances record their severity in the log.""" + + def _hard_accept(self, **overrides): + values = { + "key_value": "key1", + "action": "accept", + "check_type": "constraints", + "column": "age", + "current_value": 25, + "reason": "verified with respondent", + "severity": "hard", + } + return CorrectionEntry(**(values | overrides)) + + def test_apply_corrections_logs_the_severity(self, store, sample_data): + _seed_prep(store, sample_data) + processor = CorrectionProcessor("p1") + + processor.apply_corrections( + alias="survey", + key_col="survey_key", + entries=[self._hard_accept()], + source="constraints", + ) + + log = processor.get_correction_log("survey") + assert log.select("action", "source", "severity").rows() == [ + ("accept", "constraints", "hard") + ] + + def test_entries_without_severity_log_null(self, store, sample_data): + _seed_prep(store, sample_data) + processor = CorrectionProcessor("p1") + + processor.apply_corrections( + alias="survey", + key_col="survey_key", + entries=[self._hard_accept(severity=None)], + source="constraints", + ) + + assert processor.get_correction_log("survey")["severity"].to_list() == [None] + + def test_severity_is_only_recorded_on_acceptances(self, store, sample_data): + _seed_prep(store, sample_data) + processor = CorrectionProcessor("p1") + + with pytest.raises(ValueError, match="severity"): + processor.apply_corrections( + alias="survey", + key_col="survey_key", + entries=[ + self._hard_accept( + action="modify value", new_value=26, check_type=None + ) + ], + source="constraints", + ) + + assert processor.get_correction_log("survey").is_empty() + + def test_legacy_logs_load_with_null_severity(self, store, sample_corrections_log): + store[("p1", "logs", "corr_log_survey")] = sample_corrections_log + + log = CorrectionProcessor("p1").get_correction_log("survey") + + assert log["severity"].to_list() == [None] * 3 + + def test_summary_carries_the_severity(self, store, sample_data): + _seed_prep(store, sample_data) + processor = CorrectionProcessor("p1") + processor.apply_corrections( + alias="survey", + key_col="survey_key", + entries=[self._hard_accept()], + source="constraints", + ) + + (summary,) = processor.get_correction_summary("survey") + + assert summary["severity"] == "hard" diff --git a/tests/replication/test_package_builder.py b/tests/replication/test_package_builder.py index 97804932..a68d6e49 100644 --- a/tests/replication/test_package_builder.py +++ b/tests/replication/test_package_builder.py @@ -342,7 +342,7 @@ def _duckdb_get(project_id, table, db_name): ) assert header == ( "date,KEY,ID,action,column,current_value,new_value,reason," - "status,status_reason,source,check_type" + "status,status_reason,source,check_type,severity" ) diff --git a/tests/views/test_correction_view.py b/tests/views/test_correction_view.py index 94adeb02..ea31236c 100644 --- a/tests/views/test_correction_view.py +++ b/tests/views/test_correction_view.py @@ -5,6 +5,7 @@ from contextlib import contextmanager from unittest.mock import MagicMock, patch +import pandas as pd import polars as pl import pytest @@ -26,6 +27,7 @@ _handle_remove_correction, get_current_value, get_key_options, + highlight_hard_acceptance, load_hfc_config, load_tab_config, main, @@ -465,6 +467,7 @@ def test_status_columns_ordered_right_after_action(self): "status", "status_reason", "check_type", + "severity", "column", "current_value", "new_value", @@ -492,6 +495,40 @@ def test_accept_rows_show_their_check_type_and_source(self): ("accept", "outliers", "outliers") ] + def test_hard_violation_acceptances_show_their_severity(self): + log = self._base_log( + action=["accept"], + new_value=[None], + check_type=["constraints"], + source=["constraints"], + severity=["hard"], + ) + + result = _build_correction_log_display(log) + + assert result["severity"].to_list() == ["hard"] + + +class TestHighlightHardAcceptance: + """Hard-violation acceptances stand out in the Correction Log.""" + + def test_highlights_every_cell_of_a_hard_acceptance(self): + row = pd.Series({"action": "accept", "severity": "hard", "KEY": "k1"}) + + styles = highlight_hard_acceptance(row) + + assert len(styles) == len(row) + assert all(style and "background" in style for style in styles) + + @pytest.mark.parametrize( + ("action", "severity"), + [("accept", None), ("modify value", None), ("accept", "soft")], + ) + def test_leaves_other_rows_plain(self, action, severity): + row = pd.Series({"action": action, "severity": severity, "KEY": "k1"}) + + assert highlight_hard_acceptance(row) == ["", "", ""] + class TestLoadTabConfig: """Test that load_tab_config threads the configured Survey ID column.""" From 86c083039a4af37f8a808de615424362f4b752c6 Mon Sep 17 00:00:00 2001 From: iabaako Date: Thu, 1 Oct 2026 10:59:15 +0000 Subject: [PATCH 02/12] fix(outliers): address review of check-page corrections - Test hard constraint bounds before soft ones: a value above the hard maximum was labelled "above soft maximum" whenever a soft maximum was set, so it skipped the hard-acceptance confirmation and was undercounted - Key each results table on the rows it shows, so a selected row position never carries over to a different flag after "Show reviewed", a column change or a save - Queue the post-save toast with queue_notice (new "toast" level) per CONTRIBUTING; show_queued_notices returns whether it showed anything - Test that outlier metrics leave out accepted outliers - Rename the report's wrapper to _with_review_status; reuse _is_flagged in select_flag Refs #298 Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 9 +- CONTRIBUTING.md | 6 +- src/datasure/checks/outliers/compute.py | 6 +- src/datasure/checks/outliers/report_ui.py | 35 +++--- src/datasure/checks/outliers/review.py | 6 +- src/datasure/utils/ui_utils.py | 18 ++- tests/checks/outliers/test_compute.py | 24 ++++ .../outliers/test_report_ui_corrections.py | 113 +++++++++++++++--- tests/utils/test_ui_utils.py | 11 +- 9 files changed, 180 insertions(+), 48 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6ffccb12..166f31d9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -42,7 +42,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 the acceptance is removed. Accepting a hard violation needs a confirmation. Flag review logic lives in the new Streamlit-free `src/datasure/checks/outliers/review.py`; `outliers_report` takes the - dataset `alias`. Removed the unused `_render_outlier_table` — #298 + dataset `alias`. Removed the unused `_render_outlier_table`. + `queue_notice` gains a `toast` level, and `show_queued_notices` returns + whether it showed anything — #298 - **Correction log severity**: New `severity` column, `hard` on acceptances of hard constraint violations (null otherwise and for legacy logs). `CorrectionEntry.severity` sets it and is rejected on non-accept actions. @@ -57,6 +59,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 string still does; use "remove value" to blank a cell — #296 - **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 + violation whenever a soft bound on the same side was set (for example, + above the hard maximum read "above soft maximum"), so hard violations were + undercounted. Hard bounds are now tested first + (`compute_constraint_violations`) — #298 ## [1.1.0] - 2026-09-21 diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 13d31987..c050f7b2 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -150,9 +150,9 @@ Every Streamlit view must render its chrome through the shared helpers in (delete/remove/restart) — do not invent per-view confirm flows with session-state flags, expanders, or inline warnings. - `queue_notice(scope, level, message)` for any success/warning/error message - raised just before an `st.rerun()` (including a `confirm_dialog` callback, - which reruns), with `show_queued_notices(scope)` where it should appear on - the next run. Rendering it directly gets cleared by the rerun. + or toast raised just before an `st.rerun()` (including a `confirm_dialog` + callback, which reruns), with `show_queued_notices(scope)` where it should + appear on the next run. Rendering it directly gets cleared by the rerun. - Use `st.divider()` for horizontal rules, never `st.write("---")`. - Icons are Material shortcodes (`:material/check_circle:`), not emoji shortcodes (`:white_check_mark:`). diff --git a/src/datasure/checks/outliers/compute.py b/src/datasure/checks/outliers/compute.py index d80339ba..90ac12ee 100644 --- a/src/datasure/checks/outliers/compute.py +++ b/src/datasure/checks/outliers/compute.py @@ -920,15 +920,17 @@ def compute_constraint_violations( for col in outlier_cols: col_df = data.select([survey_key, col]) + # Hard bounds are tested before soft ones: a value past a hard + # bound is also past the soft bound inside it. violation_expr = ( pl.when((hard_min is not None) & (pl.col(col) < hard_min)) .then(pl.lit(f"Value is below hard minimum {hard_min}")) + .when((hard_max is not None) & (pl.col(col) > hard_max)) + .then(pl.lit(f"Value is above hard maximum {hard_max}")) .when((soft_min is not None) & (pl.col(col) < soft_min)) .then(pl.lit(f"Value is below soft minimum {soft_min}")) .when((soft_max is not None) & (pl.col(col) > soft_max)) .then(pl.lit(f"Value is above soft maximum {soft_max}")) - .when((hard_max is not None) & (pl.col(col) > hard_max)) - .then(pl.lit(f"Value is above hard maximum {hard_max}")) ) col_df = safe_to_numeric(col_df, col) diff --git a/src/datasure/checks/outliers/report_ui.py b/src/datasure/checks/outliers/report_ui.py index 2a896803..5a24b3f6 100644 --- a/src/datasure/checks/outliers/report_ui.py +++ b/src/datasure/checks/outliers/report_ui.py @@ -62,6 +62,7 @@ save_check_settings, trigger_save, ) +from datasure.utils.ui_utils import queue_notice, show_queued_notices # ============================================================================= # Streamlit UI - Metrics Display @@ -162,8 +163,8 @@ def _render_outlier_metrics( # Streamlit UI - Correcting and Accepting Flags # ============================================================================= -# Session-state key for the confirmation shown after the post-save rerun. -_SAVED_TOAST_KEY = "outliers_correction_toast" +# `queue_notice` scope of the confirmation shown after the post-save rerun. +_NOTICE_SCOPE = "outliers_corrections" @dataclass(frozen=True) @@ -174,7 +175,7 @@ class ReviewContext: alias: str -def _mark_reviewed( +def _with_review_status( flags: pl.DataFrame, settings: OutlierSettings, check: FlagCheck, @@ -219,11 +220,15 @@ def _render_flags_table( st.dataframe(table, **dataframe_kwargs) return - # The nonce changes after each save, which drops the old selection. + # Selection is a row position, so the key changes whenever the shown + # rows do (and after each save, via the nonce); a position never carries + # over to a different flag. nonce = st.session_state.get(_table_nonce_key(check.check_type), 0) + rows = table.select(pl.col(settings.survey_key).cast(pl.String), "column name") + fingerprint = hash(tuple(rows.hash_rows().to_list())) event = st.dataframe( table, - key=f"{check.check_type}_flags_table_{nonce}", + key=f"{check.check_type}_flags_table_{nonce}_{fingerprint}", on_select="rerun", selection_mode="single-row", **dataframe_kwargs, @@ -321,19 +326,19 @@ def _render_flag_correction_form( nonce_key = _table_nonce_key(selection.check_type) st.session_state[nonce_key] = st.session_state.get(nonce_key, 0) + 1 - st.session_state[_SAVED_TOAST_KEY] = ( + queue_notice( + _NOTICE_SCOPE, + "toast", f"Saved {state.action} on {selection.column} for KEY {key_value}. " - "It is listed in the Correction Log on the Correct Data page." + "It is listed in the Correction Log on the Correct Data page.", ) st.rerun() def _show_saved_toast() -> None: """Show the confirmation queued by a save before the page reran.""" - message = st.session_state.pop(_SAVED_TOAST_KEY, None) - if not message: + if not show_queued_notices(_NOTICE_SCOPE): return - st.toast(message, icon=":material/check_circle:") # A markdown link in the toast would open a new browser session and lose # the selected project; a page link navigates within this session. corrections_page = st.session_state.get("st_corr_page") @@ -430,7 +435,7 @@ def _render_constraint_violations_table( show_reviewed = _render_show_reviewed_toggle(CONSTRAINTS, review) violation_data = visible_flags( - _mark_reviewed(violation_data, settings, CONSTRAINTS, review), + _with_review_status(violation_data, settings, CONSTRAINTS, review), show_reviewed=show_reviewed, ) @@ -612,7 +617,7 @@ def _render_outlier_column_inspection( show_reviewed = _render_show_reviewed_toggle(OUTLIERS, review) outliers_data = visible_flags( - _mark_reviewed(outliers_data, settings, OUTLIERS, review), + _with_review_status(outliers_data, settings, OUTLIERS, review), show_reviewed=show_reviewed, ) @@ -1497,7 +1502,7 @@ def outliers_report( st.info("No constraint violations detected.") else: - constraint_violations = _mark_reviewed( + constraint_violations = _with_review_status( constraint_violations, outliers_settings, CONSTRAINTS, review ) # show constraint metrics, leaving out accepted violations @@ -1544,7 +1549,9 @@ def outliers_report( st.info("No outliers detected.") else: - outlier_data = _mark_reviewed(outlier_data, outliers_settings, OUTLIERS, review) + outlier_data = _with_review_status( + outlier_data, outliers_settings, OUTLIERS, review + ) # show outlier metrics, leaving out accepted outliers _render_outlier_metrics( clear_reviewed_flags(outlier_data, OUTLIERS), outliers_settings diff --git a/src/datasure/checks/outliers/review.py b/src/datasure/checks/outliers/review.py index dc2e4905..b4650b03 100644 --- a/src/datasure/checks/outliers/review.py +++ b/src/datasure/checks/outliers/review.py @@ -176,12 +176,14 @@ def select_flag( return None row = table.row(rows[0], named=True) - reason = row.get(check.reason_col) + flagged = check.reason_col in table.columns and bool( + table.slice(rows[0], 1).select(_is_flagged(check)).item() + ) return FlagSelection( key_value=row[survey_key], column=row["column name"], check_type=check.check_type, - flagged=reason is not None and reason != check.no_flag, + flagged=flagged, reviewed=row.get(REVIEW_STATUS_COL) is not None, hard=row.get("violation type") in _HARD_VIOLATION_TYPES, ) diff --git a/src/datasure/utils/ui_utils.py b/src/datasure/utils/ui_utils.py index 33c08ab8..6c06cf01 100644 --- a/src/datasure/utils/ui_utils.py +++ b/src/datasure/utils/ui_utils.py @@ -16,7 +16,7 @@ from dataclasses import dataclass from typing import Literal -NoticeLevel = Literal["success", "warning", "error"] +NoticeLevel = Literal["success", "warning", "error", "toast"] _QUEUED_NOTICES_KEY = "st_queued_notices" @@ -174,8 +174,9 @@ def queue_notice(scope: str, level: NoticeLevel, message: str) -> None: ---------- scope : str Where the message belongs, e.g. ``"prep_survey"`` for one Prep tab. - level : {"success", "warning", "error"} - The Streamlit callout used to render the message. + level : {"success", "warning", "error", "toast"} + The Streamlit callout used to render the message, or "toast" for a + transient ``st.toast``. message : str The message text (Markdown). """ @@ -185,10 +186,15 @@ def queue_notice(scope: str, level: NoticeLevel, message: str) -> None: queued.setdefault(scope, []).append(Notice(level, message)) -def show_queued_notices(scope: str) -> None: - """Render and clear the messages queued for a scope, in queue order.""" +def show_queued_notices(scope: str) -> bool: + """Render and clear the messages queued for a scope, in queue order. + + Returns True if any message was shown. + """ import streamlit as st queued = st.session_state.get(_QUEUED_NOTICES_KEY, {}) - for notice in queued.pop(scope, []): + notices = queued.pop(scope, []) + for notice in notices: getattr(st, notice.level)(notice.message) + return bool(notices) diff --git a/tests/checks/outliers/test_compute.py b/tests/checks/outliers/test_compute.py index 2c96c0a9..ba63b475 100644 --- a/tests/checks/outliers/test_compute.py +++ b/tests/checks/outliers/test_compute.py @@ -329,6 +329,30 @@ def test_no_bounds_set(self, sample_polars_df, outlier_settings): ) assert result.is_empty() + def test_hard_bounds_take_precedence_over_soft_bounds(self, outlier_settings): + """A value past a hard bound is a hard violation, not a soft one.""" + data = pl.DataFrame( + {"survey_key": ["K1", "K2", "K3", "K4"], "age": [150, 70, -5, 10]} + ) + config = pl.DataFrame( + { + "column_name": [["age"]], + "hard_min": [0.0], + "soft_min": [15.0], + "soft_max": [65.0], + "hard_max": [100.0], + } + ) + + result = compute_constraint_violations(data, outlier_settings, config) + + assert result["violation reason"].to_list() == [ + "Value is above hard maximum 100.0", + "Value is above soft maximum 65.0", + "Value is below hard minimum 0.0", + "Value is below soft minimum 15.0", + ] + class TestComputeColumnOutlierSummary: """Test compute_column_outlier_summary function.""" diff --git a/tests/checks/outliers/test_report_ui_corrections.py b/tests/checks/outliers/test_report_ui_corrections.py index bcbad4cd..8ca99a06 100644 --- a/tests/checks/outliers/test_report_ui_corrections.py +++ b/tests/checks/outliers/test_report_ui_corrections.py @@ -204,6 +204,34 @@ def test_outlier_acceptance_does_not_hide_a_constraint_violation( assert _shown_table(st_mock)["KEY"].to_list() == ["K1", "K2"] + def test_selection_resets_when_the_shown_rows_change( + self, data, violations, settings + ): + """A row position must never carry over to a different set of rows.""" + review = _review({"constraints": _acceptances(("K1", "age", "verified"))}) + hidden, shown = _st_mock(show_reviewed=False), _st_mock(show_reviewed=True) + + self._render(data, violations, settings, hidden, review) + self._render(data, violations, settings, shown, review) + + assert ( + hidden.dataframe.call_args.kwargs["key"] + != shown.dataframe.call_args.kwargs["key"] + ) + + def test_selection_is_kept_while_the_rows_are_unchanged( + self, data, violations, settings + ): + first, second = _st_mock(), _st_mock() + + self._render(data, violations, settings, first, _review()) + self._render(data, violations, settings, second, _review()) + + assert ( + first.dataframe.call_args.kwargs["key"] + == second.dataframe.call_args.kwargs["key"] + ) + def test_without_review_the_table_is_read_only(self, data, violations, settings): st_mock = _st_mock() @@ -325,17 +353,21 @@ def test_apply_saves_with_the_check_as_source_and_reruns(self, data, settings): st_mock.rerun.assert_called_once() def test_a_save_queues_a_toast_for_after_the_rerun(self, data, settings): - st_mock, _, _ = self._render( - data, - settings, - _selection(), - _review(), - _form_state(), - apply=True, - confirm=False, - ) + with patch(f"{MODULE}.queue_notice") as queue_notice: + self._render( + data, + settings, + _selection(), + _review(), + _form_state(), + apply=True, + confirm=False, + ) - assert any("toast" in key for key in st_mock.session_state) + scope, level, message = queue_notice.call_args.args + assert scope == "outliers_corrections" + assert level == "toast" + assert "Correction Log" in message def test_hard_violation_accept_is_disabled_until_confirmed(self, data, settings): st_mock, _, apply_entries = self._render( @@ -410,7 +442,22 @@ def test_failed_save_does_not_rerun(self, data, settings): class TestMetricsExcludeAcceptedFlags: - def test_metrics_do_not_count_accepted_flags(self, data, violations): + @pytest.fixture + def outliers(self) -> pl.DataFrame: + return pl.DataFrame( + { + "KEY": ["K1", "K2", "K3"], + "column name": ["age"] * 3, + "column value": [150.0, 70.0, 30.0], + "outlier reason": [ + "Value is above upper bound 120.00", + "Value is above upper bound 60.00", + "no outlier", + ], + } + ) + + def _run_report(self, data, violations, outliers, acceptances_by_check): config = {"survey_key": "KEY", "survey_id": "hhid"} columns = ColumnByType( all_columns=data.columns, @@ -420,12 +467,9 @@ def test_metrics_do_not_count_accepted_flags(self, data, violations): string_columns=[], integer_columns=["age"], ) - review_processor = _review( - {"constraints": _acceptances(("K1", "age", "verified"))} - ).processor - st_mock = _st_mock() + processor = _review(acceptances_by_check).processor with ( - patch(f"{MODULE}.st", st_mock), + patch(f"{MODULE}.st", _st_mock()), patch( f"{MODULE}.outliers_report_settings", return_value=OutlierSettings(**config), @@ -438,10 +482,12 @@ def test_metrics_do_not_count_accepted_flags(self, data, violations): patch(f"{MODULE}._update_unlocked_cols", side_effect=lambda df, _: df), patch(f"{MODULE}.duckdb_save_table"), patch(f"{MODULE}.compute_constraint_violations", return_value=violations), - patch(f"{MODULE}.compute_outlier_output", return_value=pl.DataFrame()), - patch(f"{MODULE}.CorrectionProcessor", return_value=review_processor), - patch(f"{MODULE}._render_constraint_metrics") as metrics, + patch(f"{MODULE}.compute_outlier_output", return_value=outliers), + patch(f"{MODULE}.CorrectionProcessor", return_value=processor), + patch(f"{MODULE}._render_constraint_metrics") as constraint_metrics, patch(f"{MODULE}._render_constraint_violations_table") as table, + patch(f"{MODULE}._render_outlier_metrics") as outlier_metrics, + patch(f"{MODULE}._render_outlier_column_inspection") as inspection, ): outliers_report( "proj1", @@ -452,9 +498,38 @@ def test_metrics_do_not_count_accepted_flags(self, data, violations): columns, alias="survey", ) + return constraint_metrics, table, outlier_metrics, inspection + + def test_constraint_metrics_do_not_count_accepted_violations( + self, data, violations, outliers + ): + metrics, table, _, _ = self._run_report( + data, + violations, + outliers, + {"constraints": _acceptances(("K1", "age", "verified"))}, + ) counted = metrics.call_args.args[0] assert counted.filter(pl.col("violation reason") != "no violation")[ "KEY" ].to_list() == ["K2"] assert table.call_args.kwargs["review"].alias == "survey" + + def test_outlier_metrics_do_not_count_accepted_outliers( + self, data, violations, outliers + ): + _, _, metrics, inspection = self._run_report( + data, + violations, + outliers, + {"outliers": _acceptances(("K2", "age", "verified"))}, + ) + + counted = metrics.call_args.args[0] + assert counted.filter(pl.col("outlier reason") != "no outlier")[ + "KEY" + ].to_list() == ["K1"] + # Rows are kept, so the column still counts as checked. + assert counted.height == outliers.height + assert inspection.call_args.kwargs["review"].alias == "survey" diff --git a/tests/utils/test_ui_utils.py b/tests/utils/test_ui_utils.py index bc84869f..234de23c 100644 --- a/tests/utils/test_ui_utils.py +++ b/tests/utils/test_ui_utils.py @@ -222,8 +222,17 @@ def test_notices_are_scoped(self, st_with_state): st_with_state.success.assert_not_called() def test_showing_with_nothing_queued_is_a_no_op(self, st_with_state): - show_queued_notices("prep_survey") + shown = show_queued_notices("prep_survey") + assert shown is False st_with_state.success.assert_not_called() st_with_state.warning.assert_not_called() st_with_state.error.assert_not_called() + + def test_toast_notices_render_as_toasts(self, st_with_state): + queue_notice("outliers", "toast", "Saved") + + shown = show_queued_notices("outliers") + + assert shown is True + st_with_state.toast.assert_called_once_with("Saved") From 230cd4eec984054ae65ef1ec348c1116b9d11b6e Mon Sep 17 00:00:00 2001 From: iabaako Date: Thu, 1 Oct 2026 11:11:21 +0000 Subject: [PATCH 03/12] feat(outliers): open flag corrections from a Review button in a dialog Replace row selection on the constraint violations and outlier inspection tables with a pinned first column of Review buttons (st.column_config.ButtonColumn). Clicking one opens the shared correction form for that row in an st.dialog instead of below the table; a save reruns the page, which closes the dialog. A button click is only present during the rerun it triggers, so the selection-reset nonce and row fingerprint in the table key are no longer needed and are removed. Refs #298 Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 7 +- docs/USER_GUIDE.md | 7 +- src/datasure/checks/outliers/report_ui.py | 165 ++++++++++-------- .../outliers/test_report_ui_corrections.py | 116 ++++++------ 4 files changed, 158 insertions(+), 137 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 166f31d9..15887691 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,9 +33,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 `apply_correction_entries`) renders the action, new-value and reason inputs for a prefilled KEY/column/current value, with namespaced widget keys. The Correct Data page now uses it — #296 -- **Outliers and constraints corrections**: Selecting a row in the constraint - violations or outlier inspection table opens the shared correction form, - prefilled with the row's KEY, column and current value, to modify the +- **Outliers and constraints corrections**: Each row of the constraint + violations and outlier inspection tables has a Review button (a pinned + `st.column_config.ButtonColumn`) that opens the shared correction form in a + dialog, prefilled with the row's KEY, column and current value, to modify the value, remove it or accept it as valid (source and check type `outliers`/`constraints`). Accepted flags are hidden and left out of the metrics unless "Show reviewed" is on, and come back if the value changes or diff --git a/docs/USER_GUIDE.md b/docs/USER_GUIDE.md index abca97dd..19f937aa 100644 --- a/docs/USER_GUIDE.md +++ b/docs/USER_GUIDE.md @@ -831,9 +831,10 @@ Visual analysis: ##### Correcting or Accepting Flagged Values -Click a row in the constraint violations table or the outlier inspection table -to open a correction form below it. The KEY, column and current value are -filled in. Choose an action, enter a reason and click "Apply": +Each row of the constraint violations table and the outlier inspection table +starts with a **Review** button. Click it to open a correction form in a +dialog, with the KEY, column and current value filled in. Choose an action, +enter a reason and click "Apply": - **modify value** or **remove value** corrects the data. The page reloads, and the flag is updated or disappears. diff --git a/src/datasure/checks/outliers/report_ui.py b/src/datasure/checks/outliers/report_ui.py index 5a24b3f6..d92a8961 100644 --- a/src/datasure/checks/outliers/report_ui.py +++ b/src/datasure/checks/outliers/report_ui.py @@ -166,6 +166,10 @@ def _render_outlier_metrics( # `queue_notice` scope of the confirmation shown after the post-save rerun. _NOTICE_SCOPE = "outliers_corrections" +# First column of a results table: a button that opens the correction dialog. +REVIEW_BUTTON_COL = "_review" +REVIEW_BUTTON_LABEL = ":material/edit_note: Review" + @dataclass(frozen=True) class ReviewContext: @@ -203,10 +207,6 @@ def _render_show_reviewed_toggle( ) -def _table_nonce_key(check_type: str) -> str: - return f"{check_type}_flags_table_nonce" - - def _render_flags_table( table: pl.DataFrame, data: pl.DataFrame, @@ -215,31 +215,46 @@ def _render_flags_table( review: ReviewContext | None, **dataframe_kwargs: Any, ) -> None: - """Render a results table; with `review`, a selected row opens the form.""" + """Render a results table; with `review`, each row has a Review button. + + Clicking Review opens the correction form for that row in a dialog. + """ if review is None: st.dataframe(table, **dataframe_kwargs) return - # Selection is a row position, so the key changes whenever the shown - # rows do (and after each save, via the nonce); a position never carries - # over to a different flag. - nonce = st.session_state.get(_table_nonce_key(check.check_type), 0) - rows = table.select(pl.col(settings.survey_key).cast(pl.String), "column name") - fingerprint = hash(tuple(rows.hash_rows().to_list())) - event = st.dataframe( - table, - key=f"{check.check_type}_flags_table_{nonce}_{fingerprint}", - on_select="rerun", - selection_mode="single-row", + click_key = f"{check.check_type}_flag_review_click" + st.dataframe( + table.select(pl.lit(REVIEW_BUTTON_LABEL).alias(REVIEW_BUTTON_COL), pl.all()), + column_config={ + REVIEW_BUTTON_COL: st.column_config.ButtonColumn( + "", + type="tertiary", + pinned=True, + key=click_key, + help="Correct the value or accept it as valid.", + ) + }, **dataframe_kwargs, ) - selection = select_flag( - table, list(event.selection.rows), settings.survey_key, check - ) - if selection is None: - st.caption("Select a row to correct the value or accept it as valid.") - return + # The click is only present during the rerun it triggers, so the dialog + # opens once per click; widgets inside the dialog rerun just the dialog. + click = st.session_state.get(click_key) + rows = [click["row"]] if click else [] + selection = select_flag(table, rows, settings.survey_key, check) + if selection is not None: + _flag_correction_dialog(data, settings, selection, review) + + +@st.dialog("Correct or accept flagged value", width="medium") +def _flag_correction_dialog( + data: pl.DataFrame, + settings: OutlierSettings, + selection: FlagSelection, + review: ReviewContext, +) -> None: + """Show the correction form for a flag in a dialog.""" _render_flag_correction_form(data, settings, selection, review) @@ -266,66 +281,64 @@ def _render_flag_correction_form( ) namespace = f"{selection.check_type}_{key_value}_{selection.column}" - with st.container(border=True): - st.markdown(f"**{selection.column}** for KEY **{key_value}**") - if selection.reviewed: - st.info( - "This flag was accepted as valid. Remove the acceptance on the " - "Correct Data page to flag it again." - ) - - state = render_correction_inputs( - data, - key_col, - str(key_value), - key_namespace=namespace, - actions=allowed_actions(selection), - column=selection.column, - current_value=current_value, - check_type=selection.check_type, - survey_id_value=survey_id_value, + st.markdown(f"**{selection.column}** for KEY **{key_value}**") + if selection.reviewed: + st.info( + "This flag was accepted as valid. Remove the acceptance on the " + "Correct Data page to flag it again." ) - hard_accept = needs_hard_confirmation(selection, state.action) - confirmed = True - if hard_accept: - st.warning( - "This value breaks a hard constraint, a bound meant to be " - "absolute. The acceptance is highlighted in the Correction Log." - ) - confirmed = st.checkbox( - "I confirm this value is correct despite the hard constraint", - key=f"correction_hard_confirm_{namespace}", - ) + state = render_correction_inputs( + data, + key_col, + str(key_value), + key_namespace=namespace, + actions=allowed_actions(selection), + column=selection.column, + current_value=current_value, + check_type=selection.check_type, + survey_id_value=survey_id_value, + ) - apply_enabled = ( - should_enable_apply_button(state.action, state.reason, state.new_value) - and not state.validation_error - and confirmed + hard_accept = needs_hard_confirmation(selection, state.action) + confirmed = True + if hard_accept: + st.warning( + "This value breaks a hard constraint, a bound meant to be " + "absolute. The acceptance is highlighted in the Correction Log." + ) + confirmed = st.checkbox( + "I confirm this value is correct despite the hard constraint", + key=f"correction_hard_confirm_{namespace}", ) - if not st.button( - label="Apply", - key=f"correction_apply_{namespace}", - width="stretch", - disabled=not apply_enabled, - type="primary", - ): - return - entry = state.to_entry() - if hard_accept: - entry = replace(entry, severity=HARD_SEVERITY) - if not apply_correction_entries( - review.processor, - review.alias, - key_col, - [entry], - source=selection.check_type, - ): - return + apply_enabled = ( + should_enable_apply_button(state.action, state.reason, state.new_value) + and not state.validation_error + and confirmed + ) + if not st.button( + label="Apply", + key=f"correction_apply_{namespace}", + width="stretch", + disabled=not apply_enabled, + type="primary", + ): + return + + entry = state.to_entry() + if hard_accept: + entry = replace(entry, severity=HARD_SEVERITY) + if not apply_correction_entries( + review.processor, + review.alias, + key_col, + [entry], + source=selection.check_type, + ): + return - nonce_key = _table_nonce_key(selection.check_type) - st.session_state[nonce_key] = st.session_state.get(nonce_key, 0) + 1 + # A full rerun closes the dialog and refreshes the tables and metrics. queue_notice( _NOTICE_SCOPE, "toast", diff --git a/tests/checks/outliers/test_report_ui_corrections.py b/tests/checks/outliers/test_report_ui_corrections.py index 8ca99a06..9aa01a15 100644 --- a/tests/checks/outliers/test_report_ui_corrections.py +++ b/tests/checks/outliers/test_report_ui_corrections.py @@ -7,6 +7,7 @@ from datasure.checks.outliers.models import OutlierSettings from datasure.checks.outliers.report_ui import ( + REVIEW_BUTTON_COL, ReviewContext, _render_constraint_violations_table, _render_flag_correction_form, @@ -86,18 +87,25 @@ def _review(acceptances_by_check: dict[str, pl.DataFrame] | None = None): return ReviewContext(processor=processor, alias="survey") -def _st_mock(selected_rows: list[int] | None = None, show_reviewed=False): +def _st_mock(clicked: tuple[str, int] | None = None, show_reviewed=False): + """A Streamlit mock; `clicked` is (check type, row) of a Review click.""" st_mock = MagicMock() st_mock.columns.side_effect = _columns_side_effect st_mock.multiselect.return_value = [] st_mock.toggle.return_value = show_reviewed st_mock.session_state = {} - st_mock.dataframe.return_value.selection.rows = selected_rows or [] + if clicked is not None: + check_type, row = clicked + st_mock.session_state[f"{check_type}_flag_review_click"] = { + "row": row, + "label": "Review", + } return st_mock def _shown_table(st_mock) -> pl.DataFrame: - return st_mock.dataframe.call_args.args[0] + """The flags shown, without the Review button column.""" + return st_mock.dataframe.call_args.args[0].drop(REVIEW_BUTTON_COL, strict=False) def _selection(**overrides) -> FlagSelection: @@ -125,38 +133,54 @@ def _form_state(action=Action.ACCEPT, reason="verified", **overrides): return CorrectionFormState(**(values | overrides)) -class TestConstraintTableSelection: +class TestConstraintTableReviewButton: def _render(self, data, violations, settings, st_mock, review): with ( patch(f"{MODULE}.st", st_mock), patch(f"{MODULE}.load_check_settings", return_value={}), patch(f"{MODULE}.save_check_settings"), - patch(f"{MODULE}._render_flag_correction_form") as form, + patch(f"{MODULE}._flag_correction_dialog") as dialog, ): _render_constraint_violations_table( data, violations, settings, "settings.json", review=review ) - return form + return dialog - def test_table_allows_single_row_selection(self, data, violations, settings): + def test_first_column_is_a_review_button_on_every_flag( + self, data, violations, settings + ): st_mock = _st_mock() self._render(data, violations, settings, st_mock, _review()) - kwargs = st_mock.dataframe.call_args.kwargs - assert kwargs["on_select"] == "rerun" - assert kwargs["selection_mode"] == "single-row" + shown = st_mock.dataframe.call_args.args[0] + assert shown.columns[0] == REVIEW_BUTTON_COL + assert all("Review" in label for label in shown[REVIEW_BUTTON_COL]) + button_config = st_mock.dataframe.call_args.kwargs["column_config"][ + REVIEW_BUTTON_COL + ] + assert button_config is st_mock.column_config.ButtonColumn.return_value + button_kwargs = st_mock.column_config.ButtonColumn.call_args.kwargs + assert button_kwargs["key"] == "constraints_flag_review_click" + assert button_kwargs["pinned"] is True + + def test_rows_are_not_selectable(self, data, violations, settings): + st_mock = _st_mock() + + self._render(data, violations, settings, st_mock, _review()) - def test_selecting_a_row_opens_the_form_prefilled_from_it( + assert "on_select" not in st_mock.dataframe.call_args.kwargs + + def test_clicking_review_opens_the_dialog_prefilled_from_the_row( self, data, violations, settings ): - st_mock = _st_mock(selected_rows=[0]) + st_mock = _st_mock(clicked=("constraints", 0)) review = _review() - form = self._render(data, violations, settings, st_mock, review) + dialog = self._render(data, violations, settings, st_mock, review) - form.assert_called_once() - _, _, selection, passed_review = form.call_args.args + dialog.assert_called_once() + _, _, selection, passed_review = dialog.call_args.args assert selection == FlagSelection( key_value="K1", column="age", @@ -167,10 +191,19 @@ def test_selecting_a_row_opens_the_form_prefilled_from_it( ) assert passed_review is review - def test_no_selection_opens_no_form(self, data, violations, settings): - form = self._render(data, violations, settings, _st_mock(), _review()) + def test_no_click_opens_no_dialog(self, data, violations, settings): + dialog = self._render(data, violations, settings, _st_mock(), _review()) + + dialog.assert_not_called() + + def test_a_click_on_the_other_table_opens_no_dialog( + self, data, violations, settings + ): + st_mock = _st_mock(clicked=("outliers", 0)) + + dialog = self._render(data, violations, settings, st_mock, _review()) - form.assert_not_called() + dialog.assert_not_called() def test_accepted_violations_are_hidden(self, data, violations, settings): st_mock = _st_mock() @@ -204,43 +237,16 @@ def test_outlier_acceptance_does_not_hide_a_constraint_violation( assert _shown_table(st_mock)["KEY"].to_list() == ["K1", "K2"] - def test_selection_resets_when_the_shown_rows_change( - self, data, violations, settings - ): - """A row position must never carry over to a different set of rows.""" - review = _review({"constraints": _acceptances(("K1", "age", "verified"))}) - hidden, shown = _st_mock(show_reviewed=False), _st_mock(show_reviewed=True) - - self._render(data, violations, settings, hidden, review) - self._render(data, violations, settings, shown, review) - - assert ( - hidden.dataframe.call_args.kwargs["key"] - != shown.dataframe.call_args.kwargs["key"] - ) - - def test_selection_is_kept_while_the_rows_are_unchanged( - self, data, violations, settings - ): - first, second = _st_mock(), _st_mock() - - self._render(data, violations, settings, first, _review()) - self._render(data, violations, settings, second, _review()) - - assert ( - first.dataframe.call_args.kwargs["key"] - == second.dataframe.call_args.kwargs["key"] - ) - - def test_without_review_the_table_is_read_only(self, data, violations, settings): + def test_without_review_the_table_has_no_button(self, data, violations, settings): st_mock = _st_mock() self._render(data, violations, settings, st_mock, None) - assert "on_select" not in st_mock.dataframe.call_args.kwargs + assert REVIEW_BUTTON_COL not in st_mock.dataframe.call_args.args[0].columns + assert "column_config" not in st_mock.dataframe.call_args.kwargs -class TestOutlierTableSelection: +class TestOutlierTableReviewButton: @pytest.fixture def outliers(self) -> pl.DataFrame: return pl.DataFrame( @@ -263,22 +269,22 @@ def _render(self, data, outliers, settings, st_mock, review): patch(f"{MODULE}.save_check_settings"), patch(f"{MODULE}._create_descriptive_stats", return_value=pl.DataFrame()), patch(f"{MODULE}._create_box_plot"), - patch(f"{MODULE}._render_flag_correction_form") as form, + patch(f"{MODULE}._flag_correction_dialog") as dialog, ): st_mock.selectbox.return_value = "age" _render_outlier_column_inspection( data, outliers, settings, "settings.json", review=review ) - return form + return dialog - def test_selecting_a_row_opens_the_form_for_outliers( + def test_clicking_review_opens_the_dialog_for_outliers( self, data, outliers, settings ): - st_mock = _st_mock(selected_rows=[0]) + st_mock = _st_mock(clicked=("outliers", 0)) - form = self._render(data, outliers, settings, st_mock, _review()) + dialog = self._render(data, outliers, settings, st_mock, _review()) - selection = form.call_args.args[2] + selection = dialog.call_args.args[2] assert selection.key_value == "K1" assert selection.column == "age" assert selection.check_type == "outliers" From 5ee089276224cb28e69ab84fe04b42f20476acca Mon Sep 17 00:00:00 2001 From: iabaako Date: Thu, 1 Oct 2026 11:28:55 +0000 Subject: [PATCH 04/12] feat(outliers): add a "Show only flagged values" toggle above the tables Add a "Show only flagged values" toggle, on by default, above the constraint violations and outlier inspection tables, next to "Show reviewed". Turning it off shows every checked value; unflagged constraint rows have a null violation type and can still be corrected from their Review button, but not accepted. The constraint table already showed only violations, so its default is unchanged. The outlier inspection table now shows only outliers by default instead of every value. Refs #298 Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 2 + docs/USER_GUIDE.md | 4 ++ src/datasure/checks/outliers/report_ui.py | 57 ++++++++++++----- src/datasure/checks/outliers/review.py | 7 ++ .../outliers/test_report_ui_corrections.py | 64 ++++++++++++++++++- tests/checks/outliers/test_review.py | 25 ++++++++ 6 files changed, 140 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 15887691..4d8621a5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,6 +41,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 `outliers`/`constraints`). Accepted flags are hidden and left out of the metrics unless "Show reviewed" is on, and come back if the value changes or the acceptance is removed. Accepting a hard violation needs a confirmation. + A "Show only flagged values" toggle (on by default) sits above both tables; + the outlier inspection table previously always listed unflagged values too. Flag review logic lives in the new Streamlit-free `src/datasure/checks/outliers/review.py`; `outliers_report` takes the dataset `alias`. Removed the unused `_render_outlier_table`. diff --git a/docs/USER_GUIDE.md b/docs/USER_GUIDE.md index 19f937aa..823195ae 100644 --- a/docs/USER_GUIDE.md +++ b/docs/USER_GUIDE.md @@ -831,6 +831,10 @@ Visual analysis: ##### Correcting or Accepting Flagged Values +Above each of the constraint violations and outlier inspection tables, +**Show only flagged values** (on by default) limits the table to flagged +values. Turn it off to see every checked value. + Each row of the constraint violations table and the outlier inspection table starts with a **Review** button. Click it to open a correction form in a dialog, with the KEY, column and current value filled in. Choose an action, diff --git a/src/datasure/checks/outliers/report_ui.py b/src/datasure/checks/outliers/report_ui.py index d92a8961..78792a13 100644 --- a/src/datasure/checks/outliers/report_ui.py +++ b/src/datasure/checks/outliers/report_ui.py @@ -39,6 +39,7 @@ FlagSelection, allowed_actions, clear_reviewed_flags, + flagged_only, mark_reviewed, needs_hard_confirmation, select_flag, @@ -194,17 +195,35 @@ def _with_review_status( return mark_reviewed(flags, acceptances, settings.survey_key, check) -def _render_show_reviewed_toggle( +def _render_table_toggles( check: FlagCheck, review: ReviewContext | None -) -> bool: - """Render the "Show reviewed" toggle for a results table.""" - if review is None: - return False - return st.toggle( - "Show reviewed", - key=f"{check.check_type}_show_reviewed", - help="Show flags accepted as valid, with the reason they were accepted.", - ) +) -> tuple[bool, bool]: + """Render the toggles above a results table. + + Returns + ------- + tuple[bool, bool] + Whether to show only flagged values, and whether to show reviewed + flags. "Show reviewed" is only offered with `review`. + """ + tc1, tc2, _ = st.columns([0.25, 0.25, 0.5]) + with tc1: + show_flagged_only = st.toggle( + "Show only flagged values", + key=f"{check.check_type}_flagged_only", + value=True, + help="Turn off to show every checked value, flagged or not.", + ) + show_reviewed = False + if review is not None: + with tc2: + show_reviewed = st.toggle( + "Show reviewed", + key=f"{check.check_type}_show_reviewed", + help="Show flags accepted as valid, with the reason they were " + "accepted.", + ) + return show_flagged_only, show_reviewed def _render_flags_table( @@ -446,11 +465,13 @@ def _render_constraint_violations_table( st.info("No constraint violations detected.") return - show_reviewed = _render_show_reviewed_toggle(CONSTRAINTS, review) + show_flagged_only, show_reviewed = _render_table_toggles(CONSTRAINTS, review) violation_data = visible_flags( _with_review_status(violation_data, settings, CONSTRAINTS, review), show_reviewed=show_reviewed, ) + if show_flagged_only: + violation_data = flagged_only(violation_data, CONSTRAINTS) all_columns = data.columns @@ -484,18 +505,18 @@ def _render_constraint_violations_table( join_key=settings.survey_key, ) - display_df = display_df.join( + violations_df = display_df.join( violation_df, on=settings.survey_key, how="inner", ) - # show only rows with violations - violations_df = display_df.filter(pl.col("violation reason") != "no violation") - # add violation type column ie. "Soft Min", "Soft Max", "Hard Min", "Hard Max" + # (null for values within bounds, shown when flagged-only is off) violation_type_expr = ( - pl.when(pl.col("violation reason").str.contains("below hard minimum")) + pl.when(pl.col("violation reason") == CONSTRAINTS.no_flag) + .then(pl.lit(None, dtype=pl.String)) + .when(pl.col("violation reason").str.contains("below hard minimum")) .then(pl.lit("Hard Min")) .when(pl.col("violation reason").str.contains("below soft minimum")) .then(pl.lit("Soft Min")) @@ -628,11 +649,13 @@ def _render_outlier_column_inspection( if inspect_display_cols: include_cols.extend(inspect_display_cols) - show_reviewed = _render_show_reviewed_toggle(OUTLIERS, review) + show_flagged_only, show_reviewed = _render_table_toggles(OUTLIERS, review) outliers_data = visible_flags( _with_review_status(outliers_data, settings, OUTLIERS, review), show_reviewed=show_reviewed, ) + if show_flagged_only: + outliers_data = flagged_only(outliers_data, OUTLIERS) # select columns to display from data display_df = data.select(include_cols) diff --git a/src/datasure/checks/outliers/review.py b/src/datasure/checks/outliers/review.py index b4650b03..47f776d4 100644 --- a/src/datasure/checks/outliers/review.py +++ b/src/datasure/checks/outliers/review.py @@ -135,6 +135,13 @@ def clear_reviewed_flags(flags: pl.DataFrame, check: FlagCheck) -> pl.DataFrame: ) +def flagged_only(flags: pl.DataFrame, check: FlagCheck) -> pl.DataFrame: + """Return the rows of `flags` that `check` flagged.""" + if check.reason_col not in flags.columns: + return flags + return flags.filter(_is_flagged(check)) + + def visible_flags(flags: pl.DataFrame, *, show_reviewed: bool) -> pl.DataFrame: """Return the rows and columns of `flags` to show in a results table. diff --git a/tests/checks/outliers/test_report_ui_corrections.py b/tests/checks/outliers/test_report_ui_corrections.py index 9aa01a15..ae8a2e81 100644 --- a/tests/checks/outliers/test_report_ui_corrections.py +++ b/tests/checks/outliers/test_report_ui_corrections.py @@ -87,12 +87,16 @@ def _review(acceptances_by_check: dict[str, pl.DataFrame] | None = None): return ReviewContext(processor=processor, alias="survey") -def _st_mock(clicked: tuple[str, int] | None = None, show_reviewed=False): +def _st_mock( + clicked: tuple[str, int] | None = None, show_reviewed=False, flagged_only=True +): """A Streamlit mock; `clicked` is (check type, row) of a Review click.""" st_mock = MagicMock() st_mock.columns.side_effect = _columns_side_effect st_mock.multiselect.return_value = [] - st_mock.toggle.return_value = show_reviewed + st_mock.toggle.side_effect = lambda label, *, key, **kwargs: ( + flagged_only if key.endswith("_flagged_only") else show_reviewed + ) st_mock.session_state = {} if clicked is not None: check_type, row = clicked @@ -237,6 +241,46 @@ def test_outlier_acceptance_does_not_hide_a_constraint_violation( assert _shown_table(st_mock)["KEY"].to_list() == ["K1", "K2"] + def test_flagged_only_toggle_is_on_by_default(self, data, violations, settings): + st_mock = _st_mock() + + self._render(data, violations, settings, st_mock, _review()) + + toggle_kwargs = { + c.kwargs["key"]: c.kwargs for c in st_mock.toggle.call_args_list + } + assert toggle_kwargs["constraints_flagged_only"]["value"] is True + + def test_turning_flagged_only_off_shows_every_checked_value( + self, data, violations, settings + ): + st_mock = _st_mock(flagged_only=False) + + self._render(data, violations, settings, st_mock, _review()) + + table = _shown_table(st_mock) + assert table.select("KEY", "violation type").rows() == [ + ("K1", "Hard Max"), + ("K2", "Soft Max"), + ("K3", None), + ] + + def test_unflagged_rows_offer_a_review_button_too(self, data, violations, settings): + st_mock = _st_mock(clicked=("constraints", 2), flagged_only=False) + + dialog = self._render(data, violations, settings, st_mock, _review()) + + selection = dialog.call_args.args[2] + assert selection.key_value == "K3" + assert selection.flagged is False + + def test_flagged_only_toggle_works_without_review(self, data, violations, settings): + st_mock = _st_mock(flagged_only=False) + + self._render(data, violations, settings, st_mock, None) + + assert _shown_table(st_mock)["KEY"].to_list() == ["K1", "K2", "K3"] + def test_without_review_the_table_has_no_button(self, data, violations, settings): st_mock = _st_mock() @@ -290,6 +334,22 @@ def test_clicking_review_opens_the_dialog_for_outliers( assert selection.check_type == "outliers" assert selection.hard is False + def test_flagged_only_shows_only_outliers(self, data, outliers, settings): + st_mock = _st_mock() + + self._render(data, outliers, settings, st_mock, _review()) + + assert _shown_table(st_mock)["KEY"].to_list() == ["K1"] + + def test_turning_flagged_only_off_shows_every_checked_value( + self, data, outliers, settings + ): + st_mock = _st_mock(flagged_only=False) + + self._render(data, outliers, settings, st_mock, _review()) + + assert _shown_table(st_mock)["KEY"].to_list() == ["K1", "K2", "K3"] + def test_accepted_outliers_are_hidden(self, data, outliers, settings): st_mock = _st_mock() review = _review({"outliers": _acceptances(("K1", "age", "verified"))}) diff --git a/tests/checks/outliers/test_review.py b/tests/checks/outliers/test_review.py index 372d746f..632acad6 100644 --- a/tests/checks/outliers/test_review.py +++ b/tests/checks/outliers/test_review.py @@ -12,6 +12,7 @@ FlagSelection, allowed_actions, clear_reviewed_flags, + flagged_only, mark_reviewed, needs_hard_confirmation, select_flag, @@ -191,6 +192,30 @@ def test_data_without_review_columns_is_unchanged(self, outlier_flags): assert clear_reviewed_flags(outlier_flags, OUTLIERS).equals(outlier_flags) +class TestFlaggedOnly: + def test_keeps_only_flagged_rows(self, outlier_flags): + result = flagged_only(outlier_flags, OUTLIERS) + + assert result["survey_key"].to_list() == ["K1", "K3", "K1"] + assert "no outlier" not in result["outlier reason"].to_list() + + def test_uses_the_check_sentinel(self): + flags = pl.DataFrame( + { + "survey_key": ["K1", "K2"], + "column name": ["age", "age"], + "violation reason": ["Value is above hard maximum 100", "no violation"], + } + ) + + assert flagged_only(flags, CONSTRAINTS)["survey_key"].to_list() == ["K1"] + + def test_data_without_the_reason_column_is_unchanged(self): + flags = pl.DataFrame({"survey_key": ["K1"]}) + + assert flagged_only(flags, OUTLIERS).equals(flags) + + class TestVisibleFlags: @pytest.fixture def marked(self, outlier_flags): From 612808e5617dfdfd67635c8e5622954cdf1d5165 Mon Sep 17 00:00:00 2001 From: iabaako Date: Thu, 1 Oct 2026 11:32:55 +0000 Subject: [PATCH 05/12] feat(outliers): highlight reviewed flags green when "Show reviewed" is on When "Show reviewed" is on, the constraint violations and outlier inspection tables are passed to st.dataframe as a pandas Styler that colours every cell of a reviewed flag light green (the success green used for log statuses), so accepted values stand out from open flags. The Review button column works unchanged on the styled table. Refs #298 Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 3 +- docs/USER_GUIDE.md | 2 +- src/datasure/checks/outliers/report_ui.py | 7 +++- src/datasure/checks/outliers/review.py | 14 ++++++++ .../outliers/test_report_ui_corrections.py | 36 ++++++++++++++++++- tests/checks/outliers/test_review.py | 19 ++++++++++ 6 files changed, 77 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4d8621a5..94ffb2a4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,7 +39,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 dialog, prefilled with the row's KEY, column and current value, to modify the value, remove it or accept it as valid (source and check type `outliers`/`constraints`). Accepted flags are hidden and left out of the - metrics unless "Show reviewed" is on, and come back if the value changes or + metrics unless "Show reviewed" is on (then they are highlighted green), and + come back if the value changes or the acceptance is removed. Accepting a hard violation needs a confirmation. A "Show only flagged values" toggle (on by default) sits above both tables; the outlier inspection table previously always listed unflagged values too. diff --git a/docs/USER_GUIDE.md b/docs/USER_GUIDE.md index 823195ae..7c8591b6 100644 --- a/docs/USER_GUIDE.md +++ b/docs/USER_GUIDE.md @@ -844,7 +844,7 @@ enter a reason and click "Apply": the flag is updated or disappears. - **accept** records that the flagged value is correct. The flag is hidden and no longer counted in the metrics. Turn on "Show reviewed" to see - accepted flags with a Reviewed badge and the reason. + accepted flags highlighted in green, with a Reviewed badge and the reason. Outlier and constraint acceptances are separate: accepting an outlier does not accept a constraint violation on the same value. Accepting a **hard** diff --git a/src/datasure/checks/outliers/report_ui.py b/src/datasure/checks/outliers/report_ui.py index 78792a13..ff679eec 100644 --- a/src/datasure/checks/outliers/report_ui.py +++ b/src/datasure/checks/outliers/report_ui.py @@ -40,6 +40,7 @@ allowed_actions, clear_reviewed_flags, flagged_only, + highlight_reviewed_row, mark_reviewed, needs_hard_confirmation, select_flag, @@ -243,8 +244,12 @@ def _render_flags_table( return click_key = f"{check.check_type}_flag_review_click" + shown = table.select(pl.lit(REVIEW_BUTTON_LABEL).alias(REVIEW_BUTTON_COL), pl.all()) + if REVIEW_STATUS_COL in shown.columns: + # "Show reviewed" is on: colour the reviewed flags green. + shown = shown.to_pandas().style.apply(highlight_reviewed_row, axis=1) st.dataframe( - table.select(pl.lit(REVIEW_BUTTON_LABEL).alias(REVIEW_BUTTON_COL), pl.all()), + shown, column_config={ REVIEW_BUTTON_COL: st.column_config.ButtonColumn( "", diff --git a/src/datasure/checks/outliers/review.py b/src/datasure/checks/outliers/review.py index 47f776d4..8cbf0eef 100644 --- a/src/datasure/checks/outliers/review.py +++ b/src/datasure/checks/outliers/review.py @@ -135,6 +135,20 @@ def clear_reviewed_flags(flags: pl.DataFrame, check: FlagCheck) -> pl.DataFrame: ) +_REVIEWED_ROW_STYLE = "background-color: rgba(25, 135, 84, 0.15)" + + +def highlight_reviewed_row(row: Any) -> list[str]: + """Style every cell of a reviewed flag green in a results table. + + Used with a pandas ``Styler`` (``df.style.apply(highlight_reviewed_row, + axis=1)``) when "Show reviewed" is on. + """ + status = row.get(REVIEW_STATUS_COL) + style = _REVIEWED_ROW_STYLE if status == REVIEWED_BADGE else "" + return [style] * len(row) + + def flagged_only(flags: pl.DataFrame, check: FlagCheck) -> pl.DataFrame: """Return the rows of `flags` that `check` flagged.""" if check.reason_col not in flags.columns: diff --git a/tests/checks/outliers/test_report_ui_corrections.py b/tests/checks/outliers/test_report_ui_corrections.py index ae8a2e81..2df80d7f 100644 --- a/tests/checks/outliers/test_report_ui_corrections.py +++ b/tests/checks/outliers/test_report_ui_corrections.py @@ -4,6 +4,7 @@ import polars as pl import pytest +from pandas.io.formats.style import Styler from datasure.checks.outliers.models import OutlierSettings from datasure.checks.outliers.report_ui import ( @@ -109,7 +110,10 @@ def _st_mock( def _shown_table(st_mock) -> pl.DataFrame: """The flags shown, without the Review button column.""" - return st_mock.dataframe.call_args.args[0].drop(REVIEW_BUTTON_COL, strict=False) + shown = st_mock.dataframe.call_args.args[0] + if isinstance(shown, Styler): + shown = pl.from_pandas(shown.data) + return shown.drop(REVIEW_BUTTON_COL, strict=False) def _selection(**overrides) -> FlagSelection: @@ -231,6 +235,36 @@ def test_show_reviewed_shows_them_with_badge_and_reason( ("K2", None, None), ] + def test_show_reviewed_colours_reviewed_rows_green( + self, data, violations, settings + ): + st_mock = _st_mock(show_reviewed=True) + review = _review({"constraints": _acceptances(("K1", "age", "verified"))}) + + self._render(data, violations, settings, st_mock, review) + + shown = st_mock.dataframe.call_args.args[0] + assert isinstance(shown, Styler) + # Styler.ctx maps (row, column) to the CSS properties applied to it. + cell_styles = shown._compute().ctx + styled_rows = {row for (row, _), props in cell_styles.items() if props} + assert styled_rows == {0} + assert all( + prop == "background-color" + for props in cell_styles.values() + for prop, _ in props + ) + + def test_without_show_reviewed_the_table_is_not_styled( + self, data, violations, settings + ): + st_mock = _st_mock() + review = _review({"constraints": _acceptances(("K1", "age", "verified"))}) + + self._render(data, violations, settings, st_mock, review) + + assert not isinstance(st_mock.dataframe.call_args.args[0], Styler) + def test_outlier_acceptance_does_not_hide_a_constraint_violation( self, data, violations, settings ): diff --git a/tests/checks/outliers/test_review.py b/tests/checks/outliers/test_review.py index 632acad6..aa654db2 100644 --- a/tests/checks/outliers/test_review.py +++ b/tests/checks/outliers/test_review.py @@ -1,5 +1,6 @@ """Tests for datasure.checks.outliers.review.""" +import pandas as pd import polars as pl import pytest @@ -13,6 +14,7 @@ allowed_actions, clear_reviewed_flags, flagged_only, + highlight_reviewed_row, mark_reviewed, needs_hard_confirmation, select_flag, @@ -192,6 +194,23 @@ def test_data_without_review_columns_is_unchanged(self, outlier_flags): assert clear_reviewed_flags(outlier_flags, OUTLIERS).equals(outlier_flags) +class TestHighlightReviewedRow: + def test_reviewed_rows_are_green_in_every_cell(self): + row = pd.Series({"KEY": "K1", REVIEW_STATUS_COL: REVIEWED_BADGE}) + + styles = highlight_reviewed_row(row) + + assert len(styles) == len(row) + assert all("background-color" in style for style in styles) + assert all("25, 135, 84" in style for style in styles) + + @pytest.mark.parametrize("status", [None, float("nan")]) + def test_other_rows_are_plain(self, status): + row = pd.Series({"KEY": "K2", REVIEW_STATUS_COL: status}) + + assert highlight_reviewed_row(row) == ["", ""] + + class TestFlaggedOnly: def test_keeps_only_flagged_rows(self, outlier_flags): result = flagged_only(outlier_flags, OUTLIERS) From 7e80d5a5ae621132f223d4c30c4fffb0251f3b65 Mon Sep 17 00:00:00 2001 From: iabaako Date: Thu, 1 Oct 2026 11:44:15 +0000 Subject: [PATCH 06/12] fix(outliers): render styled tables regardless of the global Styler limit With "Show reviewed" on, the outlier table crashed with "The dataframe has 3124 cells, but the maximum number of cells allowed to be rendered by Pandas Styler is configured to 2395". The progress, summary and missing tabs set the process-wide `styler.render.max_elements` option to fit their own tables, so whichever tab rendered last capped every other Styler. Add ui_utils.styled_dataframe, which raises the limit to fit the table (never lowering it) only for the st.dataframe call, via pd.option_context. Use it for the outlier and constraint results tables and for the Correction Log, which also renders a Styler. Refs #298 Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 5 ++- src/datasure/checks/outliers/report_ui.py | 36 ++++++++------- src/datasure/utils/ui_utils.py | 23 +++++++++- src/datasure/views/correction_view.py | 3 +- .../outliers/test_report_ui_corrections.py | 10 +++++ tests/utils/test_ui_utils.py | 45 +++++++++++++++++++ 6 files changed, 104 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 94ffb2a4..8b955213 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -48,7 +48,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 `src/datasure/checks/outliers/review.py`; `outliers_report` takes the dataset `alias`. Removed the unused `_render_outlier_table`. `queue_notice` gains a `toast` level, and `show_queued_notices` returns - whether it showed anything — #298 + whether it showed anything. New `ui_utils.styled_dataframe` renders a + pandas Styler with the `styler.render.max_elements` limit raised to fit it + for that call only, since other tabs lower the global limit to fit their own + tables; the results tables and the Correction Log use it — #298 - **Correction log severity**: New `severity` column, `hard` on acceptances of hard constraint violations (null otherwise and for legacy logs). `CorrectionEntry.severity` sets it and is rejected on non-accept actions. diff --git a/src/datasure/checks/outliers/report_ui.py b/src/datasure/checks/outliers/report_ui.py index ff679eec..87e4777c 100644 --- a/src/datasure/checks/outliers/report_ui.py +++ b/src/datasure/checks/outliers/report_ui.py @@ -64,7 +64,11 @@ save_check_settings, trigger_save, ) -from datasure.utils.ui_utils import queue_notice, show_queued_notices +from datasure.utils.ui_utils import ( + queue_notice, + show_queued_notices, + styled_dataframe, +) # ============================================================================= # Streamlit UI - Metrics Display @@ -245,22 +249,24 @@ def _render_flags_table( click_key = f"{check.check_type}_flag_review_click" shown = table.select(pl.lit(REVIEW_BUTTON_LABEL).alias(REVIEW_BUTTON_COL), pl.all()) + column_config = { + REVIEW_BUTTON_COL: st.column_config.ButtonColumn( + "", + type="tertiary", + pinned=True, + key=click_key, + help="Correct the value or accept it as valid.", + ) + } if REVIEW_STATUS_COL in shown.columns: # "Show reviewed" is on: colour the reviewed flags green. - shown = shown.to_pandas().style.apply(highlight_reviewed_row, axis=1) - st.dataframe( - shown, - column_config={ - REVIEW_BUTTON_COL: st.column_config.ButtonColumn( - "", - type="tertiary", - pinned=True, - key=click_key, - help="Correct the value or accept it as valid.", - ) - }, - **dataframe_kwargs, - ) + styled_dataframe( + shown.to_pandas().style.apply(highlight_reviewed_row, axis=1), + column_config=column_config, + **dataframe_kwargs, + ) + else: + st.dataframe(shown, column_config=column_config, **dataframe_kwargs) # The click is only present during the rerun it triggers, so the dialog # opens once per click; widgets inside the dialog rerun just the dialog. diff --git a/src/datasure/utils/ui_utils.py b/src/datasure/utils/ui_utils.py index 6c06cf01..fb187f2c 100644 --- a/src/datasure/utils/ui_utils.py +++ b/src/datasure/utils/ui_utils.py @@ -14,7 +14,10 @@ from collections.abc import Callable, Sequence from dataclasses import dataclass -from typing import Literal +from typing import TYPE_CHECKING, Any, Literal + +if TYPE_CHECKING: + from pandas.io.formats.style import Styler NoticeLevel = Literal["success", "warning", "error", "toast"] @@ -198,3 +201,21 @@ def show_queued_notices(scope: str) -> bool: for notice in notices: getattr(st, notice.level)(notice.message) return bool(notices) + + +def styled_dataframe(styler: "Styler", **dataframe_kwargs: Any) -> Any: + """Render a pandas ``Styler`` with ``st.dataframe``, whatever its size. + + Streamlit refuses to render a Styler with more cells than the global + ``styler.render.max_elements`` pandas option, which other pages lower to + fit their own tables. The limit is raised to fit this table only for the + duration of the call, then restored. + + Returns what ``st.dataframe`` returns. + """ + import pandas as pd + import streamlit as st + + limit = max(pd.get_option("styler.render.max_elements"), styler.data.size) + with pd.option_context("styler.render.max_elements", limit): + return st.dataframe(styler, **dataframe_kwargs) diff --git a/src/datasure/views/correction_view.py b/src/datasure/views/correction_view.py index 7b6a3633..2eaa741c 100644 --- a/src/datasure/views/correction_view.py +++ b/src/datasure/views/correction_view.py @@ -40,6 +40,7 @@ metric_row, page_header, section_header, + styled_dataframe, ) @@ -615,7 +616,7 @@ def render_correction_log( section_header("Correction Log") log_display = _build_correction_log_display(correction_log).to_pandas() - st.dataframe( + styled_dataframe( log_display.style.apply(highlight_hard_acceptance, axis=1).map( highlight_status, subset=["status"] ), diff --git a/tests/checks/outliers/test_report_ui_corrections.py b/tests/checks/outliers/test_report_ui_corrections.py index 2df80d7f..401cc9b8 100644 --- a/tests/checks/outliers/test_report_ui_corrections.py +++ b/tests/checks/outliers/test_report_ui_corrections.py @@ -148,6 +148,11 @@ def _render(self, data, violations, settings, st_mock, review): patch(f"{MODULE}.load_check_settings", return_value={}), patch(f"{MODULE}.save_check_settings"), patch(f"{MODULE}._flag_correction_dialog") as dialog, + # styled_dataframe imports streamlit itself; forward to the mock. + patch( + f"{MODULE}.styled_dataframe", + side_effect=lambda styler, **kw: st_mock.dataframe(styler, **kw), + ), ): _render_constraint_violations_table( data, violations, settings, "settings.json", review=review @@ -348,6 +353,11 @@ def _render(self, data, outliers, settings, st_mock, review): patch(f"{MODULE}._create_descriptive_stats", return_value=pl.DataFrame()), patch(f"{MODULE}._create_box_plot"), patch(f"{MODULE}._flag_correction_dialog") as dialog, + # styled_dataframe imports streamlit itself; forward to the mock. + patch( + f"{MODULE}.styled_dataframe", + side_effect=lambda styler, **kw: st_mock.dataframe(styler, **kw), + ), ): st_mock.selectbox.return_value = "age" _render_outlier_column_inspection( diff --git a/tests/utils/test_ui_utils.py b/tests/utils/test_ui_utils.py index 234de23c..be3e46d4 100644 --- a/tests/utils/test_ui_utils.py +++ b/tests/utils/test_ui_utils.py @@ -3,6 +3,7 @@ import sys from unittest.mock import MagicMock +import pandas as pd import pytest from datasure.utils.ui_utils import ( @@ -12,6 +13,7 @@ queue_notice, section_header, show_queued_notices, + styled_dataframe, ) @@ -236,3 +238,46 @@ def test_toast_notices_render_as_toasts(self, st_with_state): assert shown is True st_with_state.toast.assert_called_once_with("Saved") + + +class TestStyledDataframe: + """Styled tables render whatever their size, without leaking the limit.""" + + @pytest.fixture + def small_limit(self): + """Simulate another page having lowered the global Styler limit.""" + with pd.option_context("styler.render.max_elements", 10): + yield + + def test_raises_the_styler_limit_to_fit_the_table(self, mock_st, small_limit): + styler = pd.DataFrame({"a": range(50), "b": range(50)}).style + seen_limits = [] + mock_st.dataframe.side_effect = lambda *a, **k: seen_limits.append( + pd.get_option("styler.render.max_elements") + ) + + styled_dataframe(styler, width="stretch") + + assert seen_limits == [100] + mock_st.dataframe.assert_called_once_with(styler, width="stretch") + + def test_restores_the_previous_limit(self, mock_st, small_limit): + styled_dataframe(pd.DataFrame({"a": range(50)}).style) + + assert pd.get_option("styler.render.max_elements") == 10 + + def test_never_lowers_a_higher_limit(self, mock_st): + seen_limits = [] + mock_st.dataframe.side_effect = lambda *a, **k: seen_limits.append( + pd.get_option("styler.render.max_elements") + ) + + with pd.option_context("styler.render.max_elements", 1_000): + styled_dataframe(pd.DataFrame({"a": [1, 2]}).style) + + assert seen_limits == [1_000] + + def test_returns_what_st_dataframe_returns(self, mock_st): + result = styled_dataframe(pd.DataFrame({"a": [1]}).style) + + assert result is mock_st.dataframe.return_value From b9de51c102224bc7f86c489c86275309a45050c4 Mon Sep 17 00:00:00 2001 From: iabaako Date: Thu, 1 Oct 2026 12:07:54 +0000 Subject: [PATCH 07/12] fix(outliers): show corrected values under "Show reviewed" and keep values as displayed - With "Show reviewed" on, a cell whose current value comes from a modify or remove correction is highlighted green with a "Corrected" badge and the correction reason (new CorrectionProcessor.get_active_corrections: a correction is active while the data still holds its result). Unlike accepted flags, a corrected value that is still flagged stays visible and counted, and can still be accepted - Fix values changing when "Show reviewed" is on: Streamlit shows a Styler's formatted text, and pandas' defaults showed 150 as 150.000000, rounded to six decimals and missing values as "nan". New ui_utils.row_styler keeps nullable types and shows plain values; the results tables and the Correction Log use it - Row highlight and status styling ignore pd.NA instead of raising - Hide the outlier inspection table's index, like the constraint table Refs #298 Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 8 ++ docs/USER_GUIDE.md | 6 ++ src/datasure/checks/outliers/report_ui.py | 12 ++- src/datasure/checks/outliers/review.py | 91 +++++++++++++------ src/datasure/processing/corrections.py | 48 ++++++++++ src/datasure/utils/reapply_utils.py | 5 +- src/datasure/utils/ui_utils.py | 17 ++++ src/datasure/views/correction_view.py | 14 ++- .../outliers/test_report_ui_corrections.py | 50 +++++++++- tests/checks/outliers/test_review.py | 79 +++++++++++++++- tests/processing/test_corrections.py | 81 +++++++++++++++++ tests/utils/test_reapply_utils.py | 7 ++ tests/utils/test_ui_utils.py | 40 ++++++++ tests/views/test_correction_view.py | 8 +- 14 files changed, 424 insertions(+), 42 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8b955213..341d29a2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,6 +44,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 the acceptance is removed. Accepting a hard violation needs a confirmation. A "Show only flagged values" toggle (on by default) sits above both tables; the outlier inspection table previously always listed unflagged values too. + With "Show reviewed" on, values whose current value comes from a modify or + remove correction are also highlighted green, with a "Corrected" badge and + the correction reason (new `CorrectionProcessor.get_active_corrections`); + unlike accepted flags, corrected values that are still flagged stay visible + and counted. The outlier inspection table now hides its index. Styled tables + are built with new `ui_utils.row_styler`, which keeps values displayed as in + the unstyled table (pandas' default Styler formatting showed `150` as + `150.000000` and missing values as `nan`). Flag review logic lives in the new Streamlit-free `src/datasure/checks/outliers/review.py`; `outliers_report` takes the dataset `alias`. Removed the unused `_render_outlier_table`. diff --git a/docs/USER_GUIDE.md b/docs/USER_GUIDE.md index 7c8591b6..a15c1f59 100644 --- a/docs/USER_GUIDE.md +++ b/docs/USER_GUIDE.md @@ -846,6 +846,12 @@ enter a reason and click "Apply": and no longer counted in the metrics. Turn on "Show reviewed" to see accepted flags highlighted in green, with a Reviewed badge and the reason. +"Show reviewed" also highlights corrected values in green, with a Corrected +badge and the correction's reason. A corrected value that is now in range is +no longer flagged, so turn off "Show only flagged values" to see it. A +corrected value that is still flagged stays in the table and in the metrics +until it is fixed or accepted. The badge clears if the value changes again. + Outlier and constraint acceptances are separate: accepting an outlier does not accept a constraint violation on the same value. Accepting a **hard** constraint violation needs an extra confirmation. An accepted flag comes back diff --git a/src/datasure/checks/outliers/report_ui.py b/src/datasure/checks/outliers/report_ui.py index 87e4777c..9b9627a5 100644 --- a/src/datasure/checks/outliers/report_ui.py +++ b/src/datasure/checks/outliers/report_ui.py @@ -66,6 +66,7 @@ ) from datasure.utils.ui_utils import ( queue_notice, + row_styler, show_queued_notices, styled_dataframe, ) @@ -191,13 +192,16 @@ def _with_review_status( check: FlagCheck, review: ReviewContext | None, ) -> pl.DataFrame: - """Mark the flags accepted under `check`; a no-op if already marked.""" + """Mark accepted flags and corrected values; a no-op if already marked.""" if review is None or flags.is_empty() or REVIEW_STATUS_COL in flags.columns: return flags acceptances = review.processor.get_active_acceptances( review.alias, check.check_type, settings.survey_key ) - return mark_reviewed(flags, acceptances, settings.survey_key, check) + corrections = review.processor.get_active_corrections( + review.alias, settings.survey_key + ) + return mark_reviewed(flags, acceptances, settings.survey_key, check, corrections) def _render_table_toggles( @@ -261,7 +265,7 @@ def _render_flags_table( if REVIEW_STATUS_COL in shown.columns: # "Show reviewed" is on: colour the reviewed flags green. styled_dataframe( - shown.to_pandas().style.apply(highlight_reviewed_row, axis=1), + row_styler(shown, highlight_reviewed_row), column_config=column_config, **dataframe_kwargs, ) @@ -684,7 +688,7 @@ def _render_outlier_column_inspection( OUTLIERS, review, width="stretch", - hide_index=False, + hide_index=True, ) diff --git a/src/datasure/checks/outliers/review.py b/src/datasure/checks/outliers/review.py index 8cbf0eef..8e824b32 100644 --- a/src/datasure/checks/outliers/review.py +++ b/src/datasure/checks/outliers/review.py @@ -1,4 +1,4 @@ -"""Review of outlier and constraint flags against accepted values. +"""Review of outlier and constraint flags against the correction log. A flag is reviewed when the correction log holds an active acceptance for its KEY and column under the same check type (see @@ -7,6 +7,11 @@ counts. Outlier and constraint acceptances are independent: accepting an outlier does not review a constraint violation on the same cell. +A cell whose current value comes from a correction (see +`CorrectionProcessor.get_active_corrections`) is marked corrected. Corrected +values are shown with the reviewed ones, but a corrected value that is still +flagged stays visible and counted: it still needs attention. + Kept free of Streamlit so the logic can be tested without a running app. """ @@ -20,6 +25,7 @@ REVIEW_STATUS_COL = "review status" REVIEW_REASON_COL = "review reason" REVIEWED_BADGE = "Reviewed" +CORRECTED_BADGE = "Corrected" @dataclass(frozen=True) @@ -56,11 +62,24 @@ def _is_flagged(check: FlagCheck) -> pl.Expr: ) +def _latest_reason_by_cell(log_rows: pl.DataFrame, prefix: str) -> pl.DataFrame: + """Return the latest log reason per KEY and column, keyed for a join. + + The log stores KEY as text, so keys are compared as text. + """ + return log_rows.select( + pl.col("KEY").cast(pl.String).alias("_review_key"), + pl.col("column").cast(pl.String).alias("_review_column"), + pl.col("reason").cast(pl.String).alias(f"_{prefix}_reason"), + ).unique(subset=["_review_key", "_review_column"], keep="last") + + def mark_reviewed( flags: pl.DataFrame, acceptances: pl.DataFrame, survey_key: str, check: FlagCheck, + corrections: pl.DataFrame | None = None, ) -> pl.DataFrame: """Add review status and reason columns to computed flags. @@ -76,48 +95,56 @@ def mark_reviewed( The Survey KEY column in `flags`. check : FlagCheck The check that produced `flags`. + corrections : pl.DataFrame | None + The active value corrections, as returned by + `CorrectionProcessor.get_active_corrections`. Returns ------- pl.DataFrame - `flags` in the same order, plus `REVIEW_STATUS_COL` (the reviewed - badge, or null) and `REVIEW_REASON_COL` (the acceptance reason, or - null). Only flagged rows can be reviewed. + `flags` in the same order, plus `REVIEW_STATUS_COL` and + `REVIEW_REASON_COL`: `REVIEWED_BADGE` and the acceptance reason for + an accepted flag (only flagged rows can be accepted), + `CORRECTED_BADGE` and the correction reason for a corrected cell, + otherwise null. An acceptance takes precedence over a correction. """ if flags.is_empty(): return flags - # The log stores KEY as text; the latest acceptance's reason wins. - accepted = acceptances.select( - pl.col("KEY").cast(pl.String).alias("_review_key"), - pl.col("column").cast(pl.String).alias("_review_column"), - pl.lit(REVIEWED_BADGE).alias(REVIEW_STATUS_COL), - pl.col("reason").cast(pl.String).alias(REVIEW_REASON_COL), - ).unique(subset=["_review_key", "_review_column"], keep="last") + if corrections is None: + corrections = acceptances.clear() - marked = ( - flags.with_columns( - pl.col(survey_key).cast(pl.String).alias("_review_key"), - pl.col("column name").cast(pl.String).alias("_review_column"), - ) - .join( - accepted, + keyed = flags.with_columns( + pl.col(survey_key).cast(pl.String).alias("_review_key"), + pl.col("column name").cast(pl.String).alias("_review_column"), + ) + for prefix, log_rows in (("accept", acceptances), ("correct", corrections)): + keyed = keyed.join( + _latest_reason_by_cell(log_rows, prefix), on=["_review_key", "_review_column"], how="left", maintain_order="left", ) - .drop("_review_key", "_review_column") - ) - flagged = _is_flagged(check) - return marked.with_columns( - pl.when(flagged).then(pl.col(REVIEW_STATUS_COL)).alias(REVIEW_STATUS_COL), - pl.when(flagged).then(pl.col(REVIEW_REASON_COL)).alias(REVIEW_REASON_COL), - ) + accepted = _is_flagged(check) & pl.col("_accept_reason").is_not_null() + corrected = pl.col("_correct_reason").is_not_null() + return keyed.with_columns( + pl.when(accepted) + .then(pl.lit(REVIEWED_BADGE)) + .when(corrected) + .then(pl.lit(CORRECTED_BADGE)) + .alias(REVIEW_STATUS_COL), + pl.when(accepted) + .then(pl.col("_accept_reason")) + .when(corrected) + .then(pl.col("_correct_reason")) + .alias(REVIEW_REASON_COL), + ).drop("_review_key", "_review_column", "_accept_reason", "_correct_reason") def _is_reviewed() -> pl.Expr: - return pl.col(REVIEW_STATUS_COL).is_not_null() + """Whether a row is an accepted flag (not merely a corrected value).""" + return pl.col(REVIEW_STATUS_COL).fill_null("") == REVIEWED_BADGE def clear_reviewed_flags(flags: pl.DataFrame, check: FlagCheck) -> pl.DataFrame: @@ -139,14 +166,18 @@ def clear_reviewed_flags(flags: pl.DataFrame, check: FlagCheck) -> pl.DataFrame: def highlight_reviewed_row(row: Any) -> list[str]: - """Style every cell of a reviewed flag green in a results table. + """Style every cell of an accepted or corrected row green in a table. Used with a pandas ``Styler`` (``df.style.apply(highlight_reviewed_row, axis=1)``) when "Show reviewed" is on. """ status = row.get(REVIEW_STATUS_COL) - style = _REVIEWED_ROW_STYLE if status == REVIEWED_BADGE else "" - return [style] * len(row) + # Missing values may be pd.NA, which can't be used in a boolean test. + is_reviewed = isinstance(status, str) and status in ( + REVIEWED_BADGE, + CORRECTED_BADGE, + ) + return [_REVIEWED_ROW_STYLE if is_reviewed else ""] * len(row) def flagged_only(flags: pl.DataFrame, check: FlagCheck) -> pl.DataFrame: @@ -205,7 +236,7 @@ def select_flag( column=row["column name"], check_type=check.check_type, flagged=flagged, - reviewed=row.get(REVIEW_STATUS_COL) is not None, + reviewed=row.get(REVIEW_STATUS_COL) == REVIEWED_BADGE, hard=row.get("violation type") in _HARD_VIOLATION_TYPES, ) diff --git a/src/datasure/processing/corrections.py b/src/datasure/processing/corrections.py index f5f5f97e..c8c6a68c 100644 --- a/src/datasure/processing/corrections.py +++ b/src/datasure/processing/corrections.py @@ -551,6 +551,54 @@ def get_active_acceptances( ] return acceptances.filter(pl.Series(is_active, dtype=pl.Boolean)) + def get_active_corrections(self, alias: str, key_col: str) -> pl.DataFrame: + """Return the value corrections whose result the data still holds. + + A "modify value" is active while the cell holds its new value, and a + "remove value" while the cell is missing. A correction overwritten by + a later one, or whose row was removed, is inactive. + + Parameters + ---------- + alias : str + The data alias/table name + key_col : str + The Survey KEY column name + + Returns + ------- + pl.DataFrame + The active "modify value" and "remove value" rows from the + correction log, in log order + """ + log = self.get_correction_log(alias) + if log.width == 0: + return empty_correction_log() + + corrections = log.filter( + pl.col("action").is_in([Action.MODIFY_VALUE, Action.REMOVE_VALUE]) + & pl.col("column").is_not_null() + ) + if corrections.is_empty(): + return corrections + + data = self.get_corrected_data(alias) + is_active = [ + _acceptance_mismatch( + data, + key_col, + row["KEY"], + { + row["column"]: row["new_value"] + if row["action"] == Action.MODIFY_VALUE + else None + }, + ) + is None + for row in corrections.iter_rows(named=True) + ] + return corrections.filter(pl.Series(is_active, dtype=pl.Boolean)) + def apply_correction( self, alias: str, diff --git a/src/datasure/utils/reapply_utils.py b/src/datasure/utils/reapply_utils.py index b3236649..81bff109 100644 --- a/src/datasure/utils/reapply_utils.py +++ b/src/datasure/utils/reapply_utils.py @@ -20,12 +20,15 @@ class ReapplyFailure: reason: str -def highlight_status(value: str) -> str: +def highlight_status(value: object) -> str: """Style a log status cell: green text for Successful, red for Failed. Used with a pandas ``Styler`` (``df.style.map(highlight_status, subset=["status"])``) on the prep and correction Change Log tables. """ + if not isinstance(value, str): + # Missing (None or pd.NA, which can't be used in a boolean test). + return "" if value == "Failed": return "color: #dc3545; font-weight: 600" if value == "Successful": diff --git a/src/datasure/utils/ui_utils.py b/src/datasure/utils/ui_utils.py index fb187f2c..cbdd5ce2 100644 --- a/src/datasure/utils/ui_utils.py +++ b/src/datasure/utils/ui_utils.py @@ -17,6 +17,7 @@ from typing import TYPE_CHECKING, Any, Literal if TYPE_CHECKING: + import polars as pl from pandas.io.formats.style import Styler NoticeLevel = Literal["success", "warning", "error", "toast"] @@ -219,3 +220,19 @@ def styled_dataframe(styler: "Styler", **dataframe_kwargs: Any) -> Any: limit = max(pd.get_option("styler.render.max_elements"), styler.data.size) with pd.option_context("styler.render.max_elements", limit): return st.dataframe(styler, **dataframe_kwargs) + + +def row_styler(df: "pl.DataFrame", row_style: Callable[[Any], list[str]]) -> "Styler": + """Return a pandas ``Styler`` for `df` that styles each row with `row_style`. + + ``st.dataframe`` shows a Styler's formatted text, and pandas' defaults + would change the values: integers with missing values become floats, + floats are padded or rounded to six decimals, and missing values read + "nan". Here nullable types are kept and each value is shown as its plain + text ("None" if missing), so styling changes only the colours. + + `row_style` receives each row as a pandas Series; missing values are + ``pd.NA``, which must not be used in a boolean test. + """ + pandas_df = df.to_pandas(use_pyarrow_extension_array=True) + return pandas_df.style.apply(row_style, axis=1).format(str, na_rep="None") diff --git a/src/datasure/views/correction_view.py b/src/datasure/views/correction_view.py index 2eaa741c..12434e90 100644 --- a/src/datasure/views/correction_view.py +++ b/src/datasure/views/correction_view.py @@ -39,6 +39,7 @@ confirm_dialog, metric_row, page_header, + row_styler, section_header, styled_dataframe, ) @@ -581,8 +582,13 @@ def highlight_hard_acceptance(row: Any) -> list[str]: axis=1)``). Accepting a value that breaks a hard constraint overrides a bound meant to be absolute, so those rows stand out for review. """ - is_hard_accept = row.get("action") == Action.ACCEPT and ( - row.get("severity") == HARD_SEVERITY + action, severity = row.get("action"), row.get("severity") + # Missing values may be pd.NA, which can't be used in a boolean test. + is_hard_accept = ( + isinstance(action, str) + and isinstance(severity, str) + and action == Action.ACCEPT + and severity == HARD_SEVERITY ) style = "background-color: rgba(220, 53, 69, 0.15)" if is_hard_accept else "" return [style] * len(row) @@ -615,9 +621,9 @@ def render_correction_log( else: section_header("Correction Log") - log_display = _build_correction_log_display(correction_log).to_pandas() + log_display = _build_correction_log_display(correction_log) styled_dataframe( - log_display.style.apply(highlight_hard_acceptance, axis=1).map( + row_styler(log_display, highlight_hard_acceptance).map( highlight_status, subset=["status"] ), width="stretch", diff --git a/tests/checks/outliers/test_report_ui_corrections.py b/tests/checks/outliers/test_report_ui_corrections.py index 401cc9b8..4947784d 100644 --- a/tests/checks/outliers/test_report_ui_corrections.py +++ b/tests/checks/outliers/test_report_ui_corrections.py @@ -79,12 +79,19 @@ def _acceptances(*rows: tuple[str, str, str]) -> pl.DataFrame: ) -def _review(acceptances_by_check: dict[str, pl.DataFrame] | None = None): +def _review( + acceptances_by_check: dict[str, pl.DataFrame] | None = None, + corrections: pl.DataFrame | None = None, +): + """A review context; `corrections` are active value corrections.""" acceptances_by_check = acceptances_by_check or {} processor = MagicMock() processor.get_active_acceptances.side_effect = lambda alias, check_type, key_col: ( acceptances_by_check.get(check_type, _acceptances()) ) + processor.get_active_corrections.return_value = ( + _acceptances() if corrections is None else corrections + ) return ReviewContext(processor=processor, alias="survey") @@ -260,6 +267,24 @@ def test_show_reviewed_colours_reviewed_rows_green( for prop, _ in props ) + def test_corrected_value_shows_highlighted_with_show_reviewed( + self, data, violations, settings + ): + """K3 was corrected into range: no longer flagged, but reviewed.""" + st_mock = _st_mock(show_reviewed=True, flagged_only=False) + review = _review(corrections=_acceptances(("K3", "age", "typo fixed"))) + + self._render(data, violations, settings, st_mock, review) + + table = _shown_table(st_mock) + assert table.filter(pl.col("KEY") == "K3").select( + "review status", "review reason" + ).rows() == [("Corrected", "typo fixed")] + cell_styles = st_mock.dataframe.call_args.args[0]._compute().ctx + styled_rows = {row for (row, _), props in cell_styles.items() if props} + assert styled_rows == {2} + review.processor.get_active_corrections.assert_called_with("survey", "KEY") + def test_without_show_reviewed_the_table_is_not_styled( self, data, violations, settings ): @@ -378,6 +403,29 @@ def test_clicking_review_opens_the_dialog_for_outliers( assert selection.check_type == "outliers" assert selection.hard is False + def test_index_is_hidden_like_the_constraint_table(self, data, outliers, settings): + st_mock = _st_mock() + + self._render(data, outliers, settings, st_mock, _review()) + + assert st_mock.dataframe.call_args.kwargs["hide_index"] is True + + def test_show_reviewed_keeps_values_as_displayed(self, data, outliers, settings): + """Styling for "Show reviewed" must not turn 150 into 150.000000.""" + st_mock = _st_mock(show_reviewed=True, flagged_only=False) + review = _review({"outliers": _acceptances(("K1", "age", "verified"))}) + + self._render(data, outliers, settings, st_mock, review) + + styler = st_mock.dataframe.call_args.args[0] + body = styler._translate(False, False)["body"] + value_col = list(styler.data.columns).index("column value") + assert [row[value_col + 1]["display_value"] for row in body] == [ + "150.0", + "70.0", + "30.0", + ] + def test_flagged_only_shows_only_outliers(self, data, outliers, settings): st_mock = _st_mock() diff --git a/tests/checks/outliers/test_review.py b/tests/checks/outliers/test_review.py index aa654db2..6c46d0a1 100644 --- a/tests/checks/outliers/test_review.py +++ b/tests/checks/outliers/test_review.py @@ -6,6 +6,7 @@ from datasure.checks.outliers.review import ( CONSTRAINTS, + CORRECTED_BADGE, OUTLIERS, REVIEW_REASON_COL, REVIEW_STATUS_COL, @@ -204,7 +205,7 @@ def test_reviewed_rows_are_green_in_every_cell(self): assert all("background-color" in style for style in styles) assert all("25, 135, 84" in style for style in styles) - @pytest.mark.parametrize("status", [None, float("nan")]) + @pytest.mark.parametrize("status", [None, float("nan"), pd.NA]) def test_other_rows_are_plain(self, status): row = pd.Series({"KEY": "K2", REVIEW_STATUS_COL: status}) @@ -377,3 +378,79 @@ def test_only_accepting_a_hard_violation_needs_confirmation(self): assert needs_hard_confirmation(hard, Action.ACCEPT) is True assert needs_hard_confirmation(hard, Action.MODIFY_VALUE) is False assert needs_hard_confirmation(soft, Action.ACCEPT) is False + + +def _corrections(rows: list[dict]) -> pl.DataFrame: + """Active value corrections shaped like `get_active_corrections` output.""" + if not rows: + return empty_correction_log() + return pl.DataFrame( + { + "KEY": [r["KEY"] for r in rows], + "action": [r.get("action", "modify value") for r in rows], + "column": [r["column"] for r in rows], + "reason": [r.get("reason", "typo") for r in rows], + } + ) + + +class TestCorrectedValues: + """Cells whose current value comes from a correction are marked Corrected.""" + + def test_corrected_rows_get_the_corrected_badge_and_reason(self, outlier_flags): + # K2/age is no longer flagged after its correction. + corrections = _corrections([{"KEY": "K2", "column": "age", "reason": "typo"}]) + + result = mark_reviewed( + outlier_flags, _acceptances([]), "survey_key", OUTLIERS, corrections + ) + + assert result.row(1, named=True)[REVIEW_STATUS_COL] == CORRECTED_BADGE + assert result.row(1, named=True)[REVIEW_REASON_COL] == "typo" + + def test_an_acceptance_takes_precedence_over_a_correction(self, outlier_flags): + result = mark_reviewed( + outlier_flags, + _acceptances([{"KEY": "K1", "column": "age", "reason": "verified"}]), + "survey_key", + OUTLIERS, + _corrections([{"KEY": "K1", "column": "age", "reason": "typo"}]), + ) + + assert result.row(0, named=True)[REVIEW_STATUS_COL] == REVIEWED_BADGE + assert result.row(0, named=True)[REVIEW_REASON_COL] == "verified" + + def test_corrected_flags_stay_visible_and_counted(self, outlier_flags): + # K3/age was corrected but is still flagged. + marked = mark_reviewed( + outlier_flags, + _acceptances([]), + "survey_key", + OUTLIERS, + _corrections([{"KEY": "K3", "column": "age"}]), + ) + + visible = visible_flags(marked, show_reviewed=False) + counted = clear_reviewed_flags(marked, OUTLIERS) + + assert "K3" in visible["survey_key"].to_list() + assert counted["outlier reason"][2] == "Value is above upper bound 80.00" + + def test_corrected_rows_are_highlighted(self): + row = pd.Series({"KEY": "K1", REVIEW_STATUS_COL: CORRECTED_BADGE}) + + assert all(highlight_reviewed_row(row)) + + def test_a_corrected_flag_can_still_be_accepted(self, outlier_flags): + marked = mark_reviewed( + outlier_flags, + _acceptances([]), + "survey_key", + OUTLIERS, + _corrections([{"KEY": "K3", "column": "age"}]), + ) + + selection = select_flag(marked, [2], "survey_key", OUTLIERS) + + assert selection.reviewed is False + assert Action.ACCEPT in allowed_actions(selection) diff --git a/tests/processing/test_corrections.py b/tests/processing/test_corrections.py index b0e4f6ef..40f72a2b 100644 --- a/tests/processing/test_corrections.py +++ b/tests/processing/test_corrections.py @@ -1914,3 +1914,84 @@ def test_summary_carries_the_severity(self, store, sample_data): (summary,) = processor.get_correction_summary("survey") assert summary["severity"] == "hard" + + +class TestActiveCorrections: + """Value corrections whose result the data still holds.""" + + def _correct(self, processor, *entries): + processor.apply_corrections( + alias="survey", + key_col="survey_key", + entries=list(entries), + source="outliers", + ) + + def _modify(self, key, column, new_value, current_value, reason="typo"): + return CorrectionEntry( + key_value=key, + action="modify value", + column=column, + current_value=current_value, + new_value=new_value, + reason=reason, + ) + + def test_returns_modify_and_remove_value_corrections(self, store, sample_data): + _seed_prep(store, sample_data) + processor = CorrectionProcessor("p1") + self._correct( + processor, + self._modify("key1", "age", 26, 25), + CorrectionEntry( + key_value="key2", + action="remove value", + column="age", + current_value=30, + reason="impossible", + ), + ) + + active = processor.get_active_corrections("survey", "survey_key") + + assert active.select("KEY", "action", "column", "reason").rows() == [ + ("key1", "modify value", "age", "typo"), + ("key2", "remove value", "age", "impossible"), + ] + + def test_a_correction_overwritten_later_is_inactive(self, store, sample_data): + _seed_prep(store, sample_data) + processor = CorrectionProcessor("p1") + self._correct(processor, self._modify("key1", "age", 26, 25, reason="first")) + self._correct(processor, self._modify("key1", "age", 27, 26, reason="second")) + + active = processor.get_active_corrections("survey", "survey_key") + + assert active["reason"].to_list() == ["second"] + + def test_excludes_acceptances_and_removed_rows(self, store, sample_data): + _seed_prep(store, sample_data) + processor = CorrectionProcessor("p1") + self._correct( + processor, + CorrectionEntry( + key_value="key1", + action="accept", + check_type="outliers", + column="age", + current_value=25, + reason="ok", + ), + CorrectionEntry(key_value="key3", action="remove row", reason="dup"), + ) + + assert processor.get_active_corrections("survey", "survey_key").is_empty() + + def test_empty_log_returns_no_corrections(self, store, sample_data): + _seed_prep(store, sample_data) + + active = CorrectionProcessor("p1").get_active_corrections( + "survey", "survey_key" + ) + + assert active.is_empty() diff --git a/tests/utils/test_reapply_utils.py b/tests/utils/test_reapply_utils.py index 0acd0407..7d42257a 100644 --- a/tests/utils/test_reapply_utils.py +++ b/tests/utils/test_reapply_utils.py @@ -26,6 +26,13 @@ def test_successful_is_highlighted_green(self): def test_unknown_status_is_not_highlighted(self): assert highlight_status("") == "" + def test_missing_status_is_not_highlighted(self): + """A null status reaches a nullable-typed Styler as pd.NA.""" + import pandas as pd + + assert highlight_status(pd.NA) == "" + assert highlight_status(None) == "" + class TestWarnReapplyFailures: """Test the shared bulk-reapply warning banner helper.""" diff --git a/tests/utils/test_ui_utils.py b/tests/utils/test_ui_utils.py index be3e46d4..4e10fc05 100644 --- a/tests/utils/test_ui_utils.py +++ b/tests/utils/test_ui_utils.py @@ -4,6 +4,7 @@ from unittest.mock import MagicMock import pandas as pd +import polars as pl import pytest from datasure.utils.ui_utils import ( @@ -11,6 +12,7 @@ metric_row, page_header, queue_notice, + row_styler, section_header, show_queued_notices, styled_dataframe, @@ -281,3 +283,41 @@ def test_returns_what_st_dataframe_returns(self, mock_st): result = styled_dataframe(pd.DataFrame({"a": [1]}).style) assert result is mock_st.dataframe.return_value + + +class TestRowStyler: + """Styling a table must not change how its values are displayed.""" + + @staticmethod + def _display_values(styler) -> list[list[str]]: + body = styler._translate(False, False)["body"] + return [[cell["display_value"] for cell in row[1:]] for row in body] + + def test_values_display_as_in_the_unstyled_table(self): + df = pl.DataFrame( + { + "KEY": ["K1", "K2"], + "age": [150, None], + "income": [1234.5678912, 70.5], + "note": ["x", None], + } + ) + + styler = row_styler(df, lambda row: [""] * len(row)) + + assert self._display_values(styler) == [ + ["K1", "150", "1234.5678912", "x"], + ["K2", "None", "70.5", "None"], + ] + + def test_applies_the_row_style(self): + df = pl.DataFrame({"KEY": ["K1", "K2"], "flag": ["yes", None]}) + + def green_if_flagged(row): + flagged = isinstance(row["flag"], str) + return ["background-color: green" if flagged else ""] * len(row) + + cell_styles = row_styler(df, green_if_flagged)._compute().ctx + + styled_rows = {row for (row, _), props in cell_styles.items() if props} + assert styled_rows == {0} diff --git a/tests/views/test_correction_view.py b/tests/views/test_correction_view.py index ea31236c..f51f6ef7 100644 --- a/tests/views/test_correction_view.py +++ b/tests/views/test_correction_view.py @@ -522,7 +522,13 @@ def test_highlights_every_cell_of_a_hard_acceptance(self): @pytest.mark.parametrize( ("action", "severity"), - [("accept", None), ("modify value", None), ("accept", "soft")], + [ + ("accept", None), + ("modify value", None), + ("accept", "soft"), + ("accept", pd.NA), + (pd.NA, pd.NA), + ], ) def test_leaves_other_rows_plain(self, action, severity): row = pd.Series({"action": action, "severity": severity, "KEY": "k1"}) From 0469f5da4f15bb4a6adead345ca2e6c68616e162 Mon Sep 17 00:00:00 2001 From: iabaako Date: Thu, 1 Oct 2026 18:32:43 +0000 Subject: [PATCH 08/12] feat(outliers): add a "Show only reviewed" toggle above the results tables Add "Show only reviewed" next to "Show only flagged values" and "Show reviewed". It lists only accepted (Reviewed) and corrected (Corrected) rows, highlighted green, whatever the other toggles say: a value corrected into range is unflagged, so flagged-only would hide it. The other two toggles are disabled while it is on. Offered only when corrections are enabled. The toggle values are now a review.TableFilters, applied by the Streamlit-free review.filter_table. Refs #298 Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 5 +- docs/USER_GUIDE.md | 3 + src/datasure/checks/outliers/report_ui.py | 75 +++++++++++-------- src/datasure/checks/outliers/review.py | 36 +++++++++ .../outliers/test_report_ui_corrections.py | 63 +++++++++++++++- tests/checks/outliers/test_review.py | 58 ++++++++++++++ 6 files changed, 205 insertions(+), 35 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 341d29a2..8ba43d64 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -48,7 +48,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 remove correction are also highlighted green, with a "Corrected" badge and the correction reason (new `CorrectionProcessor.get_active_corrections`); unlike accepted flags, corrected values that are still flagged stay visible - and counted. The outlier inspection table now hides its index. Styled tables + and counted. A "Show only reviewed" toggle lists only accepted and + corrected rows and disables the other two toggles while on (toggle values + are a `review.TableFilters`, applied by `review.filter_table`). The outlier + inspection table now hides its index. Styled tables are built with new `ui_utils.row_styler`, which keeps values displayed as in the unstyled table (pandas' default Styler formatting showed `150` as `150.000000` and missing values as `nan`). diff --git a/docs/USER_GUIDE.md b/docs/USER_GUIDE.md index a15c1f59..a4ab6dc9 100644 --- a/docs/USER_GUIDE.md +++ b/docs/USER_GUIDE.md @@ -852,6 +852,9 @@ no longer flagged, so turn off "Show only flagged values" to see it. A corrected value that is still flagged stays in the table and in the metrics until it is fixed or accepted. The badge clears if the value changes again. +Turn on **Show only reviewed** to list only accepted and corrected values. While +it is on, the other two toggles are disabled. + Outlier and constraint acceptances are separate: accepting an outlier does not accept a constraint violation on the same value. Accepting a **hard** constraint violation needs an extra confirmation. An accepted flag comes back diff --git a/src/datasure/checks/outliers/report_ui.py b/src/datasure/checks/outliers/report_ui.py index 9b9627a5..3cf2820a 100644 --- a/src/datasure/checks/outliers/report_ui.py +++ b/src/datasure/checks/outliers/report_ui.py @@ -37,14 +37,14 @@ REVIEW_STATUS_COL, FlagCheck, FlagSelection, + TableFilters, allowed_actions, clear_reviewed_flags, - flagged_only, + filter_table, highlight_reviewed_row, mark_reviewed, needs_hard_confirmation, select_flag, - visible_flags, ) from datasure.checks.outliers.settings_ui import outliers_report_settings from datasure.processing.correction_log import HARD_SEVERITY @@ -206,33 +206,50 @@ def _with_review_status( def _render_table_toggles( check: FlagCheck, review: ReviewContext | None -) -> tuple[bool, bool]: - """Render the toggles above a results table. +) -> TableFilters: + """Render the toggles above a results table and return their values. - Returns - ------- - tuple[bool, bool] - Whether to show only flagged values, and whether to show reviewed - flags. "Show reviewed" is only offered with `review`. + "Show reviewed" and "Show only reviewed" are only offered with + `review`. While "Show only reviewed" is on, the other two toggles are + disabled, since it overrides them. """ - tc1, tc2, _ = st.columns([0.25, 0.25, 0.5]) + reviewed_only_key = f"{check.check_type}_reviewed_only" + # Read before rendering so the toggles to its left can be disabled. + reviewed_only_on = review is not None and bool( + st.session_state.get(reviewed_only_key, False) + ) + + tc1, tc2, tc3, _ = st.columns([0.22, 0.22, 0.22, 0.34]) with tc1: - show_flagged_only = st.toggle( + flagged_only_on = st.toggle( "Show only flagged values", key=f"{check.check_type}_flagged_only", value=True, + disabled=reviewed_only_on, help="Turn off to show every checked value, flagged or not.", ) - show_reviewed = False - if review is not None: - with tc2: - show_reviewed = st.toggle( - "Show reviewed", - key=f"{check.check_type}_show_reviewed", - help="Show flags accepted as valid, with the reason they were " - "accepted.", - ) - return show_flagged_only, show_reviewed + if review is None: + return TableFilters(flagged_only=flagged_only_on) + + with tc2: + show_reviewed = st.toggle( + "Show reviewed", + key=f"{check.check_type}_show_reviewed", + disabled=reviewed_only_on, + help="Also show flags accepted as valid and corrected values, " + "highlighted green, with the reason.", + ) + with tc3: + reviewed_only = st.toggle( + "Show only reviewed", + key=reviewed_only_key, + help="Show only flags accepted as valid and corrected values.", + ) + return TableFilters( + flagged_only=flagged_only_on, + show_reviewed=show_reviewed, + reviewed_only=reviewed_only, + ) def _render_flags_table( @@ -480,13 +497,11 @@ def _render_constraint_violations_table( st.info("No constraint violations detected.") return - show_flagged_only, show_reviewed = _render_table_toggles(CONSTRAINTS, review) - violation_data = visible_flags( + violation_data = filter_table( _with_review_status(violation_data, settings, CONSTRAINTS, review), - show_reviewed=show_reviewed, + _render_table_toggles(CONSTRAINTS, review), + CONSTRAINTS, ) - if show_flagged_only: - violation_data = flagged_only(violation_data, CONSTRAINTS) all_columns = data.columns @@ -664,13 +679,11 @@ def _render_outlier_column_inspection( if inspect_display_cols: include_cols.extend(inspect_display_cols) - show_flagged_only, show_reviewed = _render_table_toggles(OUTLIERS, review) - outliers_data = visible_flags( + outliers_data = filter_table( _with_review_status(outliers_data, settings, OUTLIERS, review), - show_reviewed=show_reviewed, + _render_table_toggles(OUTLIERS, review), + OUTLIERS, ) - if show_flagged_only: - outliers_data = flagged_only(outliers_data, OUTLIERS) # select columns to display from data display_df = data.select(include_cols) diff --git a/src/datasure/checks/outliers/review.py b/src/datasure/checks/outliers/review.py index 8e824b32..6a14c669 100644 --- a/src/datasure/checks/outliers/review.py +++ b/src/datasure/checks/outliers/review.py @@ -199,6 +199,42 @@ def visible_flags(flags: pl.DataFrame, *, show_reviewed: bool) -> pl.DataFrame: return flags.filter(~_is_reviewed()).drop(REVIEW_STATUS_COL, REVIEW_REASON_COL) +@dataclass(frozen=True) +class TableFilters: + """The toggles above a results table. + + Attributes + ---------- + flagged_only : bool + Show only flagged values. + show_reviewed : bool + Also show accepted flags, with the review columns. + reviewed_only : bool + Show only accepted and corrected rows, whatever the other toggles. + """ + + flagged_only: bool = True + show_reviewed: bool = False + reviewed_only: bool = False + + +def filter_table( + flags: pl.DataFrame, filters: TableFilters, check: FlagCheck +) -> pl.DataFrame: + """Return the rows and columns of marked `flags` that `filters` show.""" + if filters.reviewed_only: + # A value corrected into range is unflagged, so flagged-only and + # show-reviewed are ignored here. + if REVIEW_STATUS_COL not in flags.columns: + return flags.clear() + return flags.filter(pl.col(REVIEW_STATUS_COL).is_not_null()) + + flags = visible_flags(flags, show_reviewed=filters.show_reviewed) + if filters.flagged_only: + flags = flagged_only(flags, check) + return flags + + def select_flag( table: pl.DataFrame, rows: list[int], diff --git a/tests/checks/outliers/test_report_ui_corrections.py b/tests/checks/outliers/test_report_ui_corrections.py index 4947784d..a1638c64 100644 --- a/tests/checks/outliers/test_report_ui_corrections.py +++ b/tests/checks/outliers/test_report_ui_corrections.py @@ -96,16 +96,28 @@ def _review( def _st_mock( - clicked: tuple[str, int] | None = None, show_reviewed=False, flagged_only=True + clicked: tuple[str, int] | None = None, + show_reviewed=False, + flagged_only=True, + reviewed_only=False, ): """A Streamlit mock; `clicked` is (check type, row) of a Review click.""" st_mock = MagicMock() st_mock.columns.side_effect = _columns_side_effect st_mock.multiselect.return_value = [] - st_mock.toggle.side_effect = lambda label, *, key, **kwargs: ( - flagged_only if key.endswith("_flagged_only") else show_reviewed + toggle_values = { + "_flagged_only": flagged_only, + "_show_reviewed": show_reviewed, + "_reviewed_only": reviewed_only, + } + st_mock.toggle.side_effect = lambda label, *, key, **kwargs: next( + value for suffix, value in toggle_values.items() if key.endswith(suffix) ) st_mock.session_state = {} + # Widget state is in session_state before the widget renders. + for suffix, value in toggle_values.items(): + for check_type in ("constraints", "outliers"): + st_mock.session_state[f"{check_type}{suffix}"] = value if clicked is not None: check_type, row = clicked st_mock.session_state[f"{check_type}_flag_review_click"] = { @@ -285,6 +297,51 @@ def test_corrected_value_shows_highlighted_with_show_reviewed( assert styled_rows == {2} review.processor.get_active_corrections.assert_called_with("survey", "KEY") + def test_show_only_reviewed_lists_accepted_and_corrected_rows( + self, data, violations, settings + ): + st_mock = _st_mock(reviewed_only=True) + review = _review( + {"constraints": _acceptances(("K1", "age", "verified"))}, + corrections=_acceptances(("K3", "age", "typo fixed")), + ) + + self._render(data, violations, settings, st_mock, review) + + table = _shown_table(st_mock) + assert table.select("KEY", "review status").rows() == [ + ("K1", "Reviewed"), + ("K3", "Corrected"), + ] + assert isinstance(st_mock.dataframe.call_args.args[0], Styler) + + def test_show_only_reviewed_disables_the_other_toggles( + self, data, violations, settings + ): + st_mock = _st_mock(reviewed_only=True) + + self._render(data, violations, settings, st_mock, _review()) + + disabled = { + c.kwargs["key"]: c.kwargs.get("disabled", False) + for c in st_mock.toggle.call_args_list + } + assert disabled == { + "constraints_flagged_only": True, + "constraints_show_reviewed": True, + "constraints_reviewed_only": False, + } + + def test_without_review_there_is_no_show_only_reviewed_toggle( + self, data, violations, settings + ): + st_mock = _st_mock() + + self._render(data, violations, settings, st_mock, None) + + keys = [c.kwargs["key"] for c in st_mock.toggle.call_args_list] + assert keys == ["constraints_flagged_only"] + def test_without_show_reviewed_the_table_is_not_styled( self, data, violations, settings ): diff --git a/tests/checks/outliers/test_review.py b/tests/checks/outliers/test_review.py index 6c46d0a1..74823d4b 100644 --- a/tests/checks/outliers/test_review.py +++ b/tests/checks/outliers/test_review.py @@ -12,8 +12,10 @@ REVIEW_STATUS_COL, REVIEWED_BADGE, FlagSelection, + TableFilters, allowed_actions, clear_reviewed_flags, + filter_table, flagged_only, highlight_reviewed_row, mark_reviewed, @@ -454,3 +456,59 @@ def test_a_corrected_flag_can_still_be_accepted(self, outlier_flags): assert selection.reviewed is False assert Action.ACCEPT in allowed_actions(selection) + + +class TestFilterTable: + """The table toggles applied together.""" + + @pytest.fixture + def marked(self, outlier_flags): + # K1/age accepted; K2/age corrected into range (unflagged). + return mark_reviewed( + outlier_flags, + _acceptances([{"KEY": "K1", "column": "age", "reason": "ok"}]), + "survey_key", + OUTLIERS, + _corrections([{"KEY": "K2", "column": "age", "reason": "typo"}]), + ) + + @staticmethod + def _cells(table): + return list(zip(table["survey_key"], table["column name"], strict=True)) + + def test_defaults_show_unreviewed_flags_only(self, marked): + result = filter_table(marked, TableFilters(), OUTLIERS) + + assert self._cells(result) == [("K3", "age"), ("K1", "income")] + assert REVIEW_STATUS_COL not in result.columns + + def test_show_reviewed_adds_accepted_flags(self, marked): + result = filter_table(marked, TableFilters(show_reviewed=True), OUTLIERS) + + assert self._cells(result) == [("K1", "age"), ("K3", "age"), ("K1", "income")] + + def test_all_values_with_reviewed(self, marked): + filters = TableFilters(flagged_only=False, show_reviewed=True) + + assert filter_table(marked, filters, OUTLIERS).height == marked.height + + def test_reviewed_only_shows_accepted_and_corrected_rows(self, marked): + result = filter_table(marked, TableFilters(reviewed_only=True), OUTLIERS) + + assert self._cells(result) == [("K1", "age"), ("K2", "age")] + assert result[REVIEW_STATUS_COL].to_list() == [REVIEWED_BADGE, CORRECTED_BADGE] + + def test_reviewed_only_overrides_the_other_toggles(self, marked): + filters = TableFilters( + flagged_only=True, show_reviewed=False, reviewed_only=True + ) + + result = filter_table(marked, filters, OUTLIERS) + + assert ("K2", "age") in self._cells(result) + + def test_reviewed_only_without_review_columns_shows_nothing(self, outlier_flags): + result = filter_table(outlier_flags, TableFilters(reviewed_only=True), OUTLIERS) + + assert result.is_empty() + assert result.columns == outlier_flags.columns From d008de5fb04858d19ef042dbd9516b0d85d831ed Mon Sep 17 00:00:00 2001 From: iabaako Date: Thu, 1 Oct 2026 19:14:00 +0000 Subject: [PATCH 09/12] fix(outliers): address Copilot review of check-page corrections - Numeric KEYs: the Review dialog passed the KEY to the shared form as text, so modify/remove on KEY 7 failed validation with "Key value '7' not found in data". The saved entry now carries the KEY's native value; the form still shows it as text - Float32 columns: a modification such as 70.1 is stored as 70.0999984741211, so get_active_corrections never matched it and the Corrected badge was missing. Compare against the logged value cast to the column's type, as _apply_modify_value does - Performance: get_active_corrections filtered the whole dataset once per logged correction. It now checks only the rows of logged KEYs (keeping every row of a duplicated KEY), and the report looks it up once per run instead of once per table - Widget keys: the dialog namespace joined check, KEY and column with underscores, so KEY "survey_1"/column "age" collided with KEY "survey"/ column "1_age". Encode the parts as JSON Refs #298 Co-Authored-By: Claude Opus 5.5 --- src/datasure/checks/outliers/report_ui.py | 32 +++++++-- src/datasure/processing/corrections.py | 33 +++++++-- .../outliers/test_report_ui_corrections.py | 44 ++++++++++++ tests/processing/test_corrections.py | 68 +++++++++++++++++++ 4 files changed, 165 insertions(+), 12 deletions(-) diff --git a/src/datasure/checks/outliers/report_ui.py b/src/datasure/checks/outliers/report_ui.py index 3cf2820a..f9b78b45 100644 --- a/src/datasure/checks/outliers/report_ui.py +++ b/src/datasure/checks/outliers/report_ui.py @@ -1,7 +1,8 @@ """Report-rendering UI for the outliers report.""" +import json from collections.abc import Callable -from dataclasses import dataclass, replace +from dataclasses import dataclass, field, replace from typing import Any import polars as pl @@ -180,10 +181,25 @@ def _render_outlier_metrics( @dataclass(frozen=True) class ReviewContext: - """What the results tables need to correct or accept flagged values.""" + """What the results tables need to correct or accept flagged values. + + One context is built per report run, so lookups shared by both tables + are cached on it. + """ processor: CorrectionProcessor alias: str + _corrections: dict[str, pl.DataFrame] = field( + default_factory=dict, compare=False, repr=False + ) + + def active_corrections(self, key_col: str) -> pl.DataFrame: + """Return the active value corrections, looked up once per run.""" + if key_col not in self._corrections: + self._corrections[key_col] = self.processor.get_active_corrections( + self.alias, key_col + ) + return self._corrections[key_col] def _with_review_status( @@ -198,9 +214,7 @@ def _with_review_status( acceptances = review.processor.get_active_acceptances( review.alias, check.check_type, settings.survey_key ) - corrections = review.processor.get_active_corrections( - review.alias, settings.survey_key - ) + corrections = review.active_corrections(settings.survey_key) return mark_reviewed(flags, acceptances, settings.survey_key, check, corrections) @@ -330,7 +344,9 @@ def _render_flag_correction_form( if settings.survey_id else None ) - namespace = f"{selection.check_type}_{key_value}_{selection.column}" + # JSON keeps the parts distinct: KEY "a_1"/column "b" and KEY "a"/column + # "1_b" would collide if joined with underscores. + namespace = json.dumps([selection.check_type, str(key_value), selection.column]) st.markdown(f"**{selection.column}** for KEY **{key_value}**") if selection.reviewed: @@ -377,7 +393,9 @@ def _render_flag_correction_form( ): return - entry = state.to_entry() + # The form holds KEY as text for display; validation and the data + # compare the KEY's native value (e.g. 7, not "7"). + entry = replace(state.to_entry(), key_value=key_value) if hard_accept: entry = replace(entry, severity=HARD_SEVERITY) if not apply_correction_entries( diff --git a/src/datasure/processing/corrections.py b/src/datasure/processing/corrections.py index c8c6a68c..9ca1e7e5 100644 --- a/src/datasure/processing/corrections.py +++ b/src/datasure/processing/corrections.py @@ -138,6 +138,26 @@ def _acceptance_is_active( ) +def _stored_correction_value(data: pl.DataFrame, row: dict[str, Any]) -> str | None: + """Return the value a logged value correction left in the data, as logged. + + "remove value" leaves the cell missing. "modify value" stores its new + value cast to the column's type, as `_apply_modify_value` does, so a + Float32 column holds 70.0999984741211 for a logged "70.1". If the cast + fails, the logged text is returned unchanged. + """ + if row["action"] != Action.MODIFY_VALUE: + return None + new_value, column = row["new_value"], row["column"] + if new_value is None or column not in data.columns: + return new_value + try: + typed = pl.select(pl.lit(new_value).cast(data.schema[column])).item() + except pl.exceptions.PolarsError: + return new_value + return _encode_scalar(typed) + + def _check_acceptance_against_data( data: pl.DataFrame, key_col: str, @@ -583,16 +603,19 @@ def get_active_corrections(self, alias: str, key_col: str) -> pl.DataFrame: return corrections data = self.get_corrected_data(alias) + if key_col in data.columns: + # Check only the rows of logged KEYs, not the whole dataset per + # correction. Every row of a duplicated KEY is kept, so all of + # them must still hold the value. + logged_keys = corrections["KEY"].unique() + data = data.filter(pl.col(key_col).cast(pl.String).is_in(logged_keys)) + is_active = [ _acceptance_mismatch( data, key_col, row["KEY"], - { - row["column"]: row["new_value"] - if row["action"] == Action.MODIFY_VALUE - else None - }, + {row["column"]: _stored_correction_value(data, row)}, ) is None for row in corrections.iter_rows(named=True) diff --git a/tests/checks/outliers/test_report_ui_corrections.py b/tests/checks/outliers/test_report_ui_corrections.py index a1638c64..c690daa6 100644 --- a/tests/checks/outliers/test_report_ui_corrections.py +++ b/tests/checks/outliers/test_report_ui_corrections.py @@ -643,6 +643,42 @@ def test_modifying_a_hard_violation_needs_no_confirmation(self, data, settings): st_mock.checkbox.assert_not_called() + def test_numeric_keys_are_saved_with_their_native_type(self, settings): + """The form shows KEY 7 as text, but validation compares native values.""" + numeric = pl.DataFrame({"KEY": [7, 8], "hhid": ["H7", "H8"], "age": [150, 30]}) + + _, inputs, apply_entries = self._render( + numeric, + settings, + _selection(key_value=7), + _review(), + _form_state(key_value="7", action=Action.MODIFY_VALUE, new_value="90"), + apply=True, + confirm=False, + ) + + assert inputs.call_args.args[2] == "7" + assert inputs.call_args.kwargs["current_value"] == 150 + (entry,) = apply_entries.call_args.args[3] + assert entry.key_value == 7 + + def test_widget_keys_are_unique_per_cell(self, data, settings): + """KEY "survey_1"/column "age" and KEY "survey"/column "1_age" differ.""" + namespaces = [] + for key, column in (("survey_1", "age"), ("survey", "1_age")): + _, inputs, _ = self._render( + data, + settings, + _selection(key_value=key, column=column), + _review(), + _form_state(key_value=key, column=column), + apply=False, + confirm=False, + ) + namespaces.append(inputs.call_args.kwargs["key_namespace"]) + + assert namespaces[0] != namespaces[1] + def test_failed_save_does_not_rerun(self, data, settings): st_mock = _st_mock() st_mock.button.return_value = True @@ -713,6 +749,7 @@ def _run_report(self, data, violations, outliers, acceptances_by_check): columns, alias="survey", ) + self.processor = processor return constraint_metrics, table, outlier_metrics, inspection def test_constraint_metrics_do_not_count_accepted_violations( @@ -748,3 +785,10 @@ def test_outlier_metrics_do_not_count_accepted_outliers( # Rows are kept, so the column still counts as checked. assert counted.height == outliers.height assert inspection.call_args.kwargs["review"].alias == "survey" + + def test_corrections_are_looked_up_once_per_report_run( + self, data, violations, outliers + ): + self._run_report(data, violations, outliers, {}) + + assert self.processor.get_active_corrections.call_count == 1 diff --git a/tests/processing/test_corrections.py b/tests/processing/test_corrections.py index 40f72a2b..1b39bbe2 100644 --- a/tests/processing/test_corrections.py +++ b/tests/processing/test_corrections.py @@ -1995,3 +1995,71 @@ def test_empty_log_returns_no_corrections(self, store, sample_data): ) assert active.is_empty() + + +class TestActiveCorrectionsMatchStoredValues: + """A correction is active if the cell holds the value it stored.""" + + @staticmethod + def _modify(key, new_value, current_value): + return CorrectionEntry( + key_value=key, + action="modify value", + column="age", + current_value=current_value, + new_value=new_value, + reason="typo", + ) + + def test_float32_modification_is_active(self, store): + """70.1 is stored as 70.0999984741211 in a Float32 column.""" + _seed_prep( + store, + pl.DataFrame( + {"KEY": ["a", "b"], "age": [150.0, 30.0]}, + schema={"KEY": pl.String, "age": pl.Float32}, + ), + ) + processor = CorrectionProcessor("p1") + processor.apply_corrections( + "survey", "KEY", [self._modify("a", "70.1", 150.0)], source="outliers" + ) + + active = processor.get_active_corrections("survey", "KEY") + + assert active["KEY"].to_list() == ["a"] + + def test_numeric_key_correction_applies_and_is_active(self, store): + _seed_prep(store, pl.DataFrame({"KEY": [7, 8], "age": [150, 30]})) + processor = CorrectionProcessor("p1") + + processor.apply_corrections( + "survey", "KEY", [self._modify(7, "90", 150)], source="outliers" + ) + + assert processor.get_corrected_data("survey")["age"].to_list() == [90, 30] + active = processor.get_active_corrections("survey", "KEY") + assert active["KEY"].to_list() == ["7"] + + def test_duplicate_keys_must_all_hold_the_value(self, store): + _seed_prep( + store, + pl.DataFrame({"KEY": ["a", "a", "b"], "age": [150, 150, 30]}), + ) + processor = CorrectionProcessor("p1") + processor.apply_corrections( + "survey", "KEY", [self._modify("a", "90", 150)], source="outliers" + ) + # Another step changes one of the duplicate rows afterwards. + data = processor.get_corrected_data("survey") + processor.save_corrected_data( + "survey", + data.with_columns( + pl.when(pl.int_range(pl.len()) == 1) + .then(pl.lit(91)) + .otherwise(pl.col("age")) + .alias("age") + ), + ) + + assert processor.get_active_corrections("survey", "KEY").is_empty() From 9c5785033212ea37b7984b1528e92dc470ba1ebb Mon Sep 17 00:00:00 2001 From: iabaako Date: Thu, 1 Oct 2026 19:34:59 +0000 Subject: [PATCH 10/12] fix(outliers): address second Copilot review of check-page corrections - Hard violations need a hard acceptance: a value accepted as a soft violation stayed accepted after bounds were tightened into a hard violation, hiding it from the table and metrics without the hard confirmation. An acceptance now covers a hard violation only if it was logged with severity "hard"; otherwise the flag stays pending and can be accepted through the confirmation - Review button column: a survey field named "_review" added via "Show more columns" duplicated the button column and Polars raised. The button column now takes a name not already in the table - Styler limit: replace the temporary pd.option_context in styled_dataframe with ensure_styler_limit, which raises the process-wide styler.render.max_elements under a lock and never lowers it, so concurrent sessions can't restore a limit below what another render needs. The summary, missing and progress checks now use it instead of pd.set_option, which lowered the limit to fit their own tables Refs #298 Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 14 +++- CONTRIBUTING.md | 4 + src/datasure/checks/missing.py | 5 +- src/datasure/checks/outliers/report_ui.py | 8 +- src/datasure/checks/outliers/review.py | 50 ++++++++++-- src/datasure/checks/progress.py | 10 +-- src/datasure/checks/summary.py | 3 +- src/datasure/utils/ui_utils.py | 35 +++++--- .../outliers/test_report_ui_corrections.py | 60 ++++++++++++-- tests/checks/outliers/test_review.py | 79 ++++++++++++++++++- tests/utils/test_ui_utils.py | 51 +++++++++--- 11 files changed, 268 insertions(+), 51 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8ba43d64..c1babba0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -59,10 +59,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 `src/datasure/checks/outliers/review.py`; `outliers_report` takes the dataset `alias`. Removed the unused `_render_outlier_table`. `queue_notice` gains a `toast` level, and `show_queued_notices` returns - whether it showed anything. New `ui_utils.styled_dataframe` renders a - pandas Styler with the `styler.render.max_elements` limit raised to fit it - for that call only, since other tabs lower the global limit to fit their own - tables; the results tables and the Correction Log use it — #298 + whether it showed anything. New `ui_utils.ensure_styler_limit` raises + pandas' process-wide `styler.render.max_elements` under a lock and never + lowers it, so concurrent sessions can't cut it below what another render + needs; it replaces the `pd.set_option` calls in the summary, missing and + progress checks, which lowered the limit to fit their own tables and could + crash other styled tables. New `ui_utils.styled_dataframe` renders a Styler + after raising the limit to fit it; the results tables and the Correction + Log use it. A soft-violation acceptance no longer hides a value that has + since become a hard violation (e.g. after bounds are tightened): it needs a + new hard acceptance — #298 - **Correction log severity**: New `severity` column, `hard` on acceptances of hard constraint violations (null otherwise and for legacy logs). `CorrectionEntry.severity` sets it and is rejected on non-accept actions. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index c050f7b2..a9135bde 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -153,6 +153,10 @@ Every Streamlit view must render its chrome through the shared helpers in or toast raised just before an `st.rerun()` (including a `confirm_dialog` callback, which reruns), with `show_queued_notices(scope)` where it should appear on the next run. Rendering it directly gets cleared by the rerun. +- Render a pandas Styler with `styled_dataframe`, or call + `ensure_styler_limit(cells)` before rendering it. Never set + `styler.render.max_elements` with `pd.set_option`: the option is shared by + every session, and lowering it can break another session's styled table. - Use `st.divider()` for horizontal rules, never `st.write("---")`. - Icons are Material shortcodes (`:material/check_circle:`), not emoji shortcodes (`:white_check_mark:`). diff --git a/src/datasure/checks/missing.py b/src/datasure/checks/missing.py index 07838e22..a573004b 100644 --- a/src/datasure/checks/missing.py +++ b/src/datasure/checks/missing.py @@ -29,6 +29,7 @@ save_check_settings, trigger_save, ) +from datasure.utils.ui_utils import ensure_styler_limit TAB_NAME = "missing" @@ -930,7 +931,7 @@ def missing_columns( if not mv_data_filtered.empty: cmap = sns.light_palette("pink", as_cmap=True) styler_limit = mv_data_filtered.shape[0] * mv_data_filtered.shape[1] - pd.set_option("styler.render.max_elements", styler_limit) + ensure_styler_limit(styler_limit) st.dataframe( mv_data_filtered.style.format( @@ -1114,7 +1115,7 @@ def missing_compare( else: cmap = sns.light_palette("pink", as_cmap=True) styler_limit = group_by_data.shape[0] * group_by_data.shape[1] - pd.set_option("styler.render.max_elements", styler_limit) + ensure_styler_limit(styler_limit) st.dataframe( group_by_data.style.format(subset=compare_col, precision=2) diff --git a/src/datasure/checks/outliers/report_ui.py b/src/datasure/checks/outliers/report_ui.py index f9b78b45..5057b251 100644 --- a/src/datasure/checks/outliers/report_ui.py +++ b/src/datasure/checks/outliers/report_ui.py @@ -283,9 +283,13 @@ def _render_flags_table( return click_key = f"{check.check_type}_flag_review_click" - shown = table.select(pl.lit(REVIEW_BUTTON_LABEL).alias(REVIEW_BUTTON_COL), pl.all()) + # Survey fields can be added to the table, so avoid their names. + button_col = REVIEW_BUTTON_COL + while button_col in table.columns: + button_col = f"_{button_col}" + shown = table.select(pl.lit(REVIEW_BUTTON_LABEL).alias(button_col), pl.all()) column_config = { - REVIEW_BUTTON_COL: st.column_config.ButtonColumn( + button_col: st.column_config.ButtonColumn( "", type="tertiary", pinned=True, diff --git a/src/datasure/checks/outliers/review.py b/src/datasure/checks/outliers/review.py index 6a14c669..79c13a40 100644 --- a/src/datasure/checks/outliers/review.py +++ b/src/datasure/checks/outliers/review.py @@ -20,7 +20,7 @@ import polars as pl -from datasure.processing.correction_log import Action +from datasure.processing.correction_log import HARD_SEVERITY, Action REVIEW_STATUS_COL = "review status" REVIEW_REASON_COL = "review reason" @@ -62,18 +62,37 @@ def _is_flagged(check: FlagCheck) -> pl.Expr: ) -def _latest_reason_by_cell(log_rows: pl.DataFrame, prefix: str) -> pl.DataFrame: - """Return the latest log reason per KEY and column, keyed for a join. +def _latest_entry_by_cell(log_rows: pl.DataFrame, prefix: str) -> pl.DataFrame: + """Return the latest log reason and severity per KEY and column, for a join. - The log stores KEY as text, so keys are compared as text. + The log stores KEY as text, so keys are compared as text. Logs without + a severity column have a null severity. """ + severity = ( + pl.col("severity").cast(pl.String) + if "severity" in log_rows.columns + else pl.lit(None, dtype=pl.String) + ) return log_rows.select( pl.col("KEY").cast(pl.String).alias("_review_key"), pl.col("column").cast(pl.String).alias("_review_column"), pl.col("reason").cast(pl.String).alias(f"_{prefix}_reason"), + severity.alias(f"_{prefix}_severity"), ).unique(subset=["_review_key", "_review_column"], keep="last") +def _is_hard_violation(check: FlagCheck) -> pl.Expr: + """Whether a row's flag is a hard constraint violation. + + Matches the reasons written by `compute_constraint_violations`. + """ + return ( + pl.col(check.reason_col) + .fill_null("") + .str.contains("below hard minimum|above hard maximum") + ) + + def mark_reviewed( flags: pl.DataFrame, acceptances: pl.DataFrame, @@ -120,13 +139,23 @@ def mark_reviewed( ) for prefix, log_rows in (("accept", acceptances), ("correct", corrections)): keyed = keyed.join( - _latest_reason_by_cell(log_rows, prefix), + _latest_entry_by_cell(log_rows, prefix), on=["_review_key", "_review_column"], how="left", maintain_order="left", ) - accepted = _is_flagged(check) & pl.col("_accept_reason").is_not_null() + # An acceptance covers a hard violation only if it was confirmed as one: + # a value accepted as a soft violation can become hard when bounds are + # tightened, and must then be confirmed again. + accepted = ( + _is_flagged(check) + & pl.col("_accept_reason").is_not_null() + & ( + ~_is_hard_violation(check) + | (pl.col("_accept_severity").fill_null("") == HARD_SEVERITY) + ) + ) corrected = pl.col("_correct_reason").is_not_null() return keyed.with_columns( pl.when(accepted) @@ -139,7 +168,14 @@ def mark_reviewed( .when(corrected) .then(pl.col("_correct_reason")) .alias(REVIEW_REASON_COL), - ).drop("_review_key", "_review_column", "_accept_reason", "_correct_reason") + ).drop( + "_review_key", + "_review_column", + "_accept_reason", + "_accept_severity", + "_correct_reason", + "_correct_severity", + ) def _is_reviewed() -> pl.Expr: diff --git a/src/datasure/checks/progress.py b/src/datasure/checks/progress.py index 6a31d8e9..799b8fd9 100644 --- a/src/datasure/checks/progress.py +++ b/src/datasure/checks/progress.py @@ -10,7 +10,6 @@ from typing import Any, Literal -import pandas as pd import plotly.graph_objects as go import polars as pl import seaborn as sns @@ -26,11 +25,12 @@ save_check_settings, trigger_save, ) +from datasure.utils.ui_utils import ensure_styler_limit TAB_NAME = "progress" -# Configure pandas styler for large dataframes (performance optimization) -pd.set_option("styler.render.max_elements", 1_000_000) +# Allow styling large dataframes (the limit is shared and only raised) +ensure_styler_limit(1_000_000) # ============================================================================= @@ -1318,8 +1318,8 @@ def _display_chart_and_table( with ai2: # Convert to pandas for styling (Streamlit doesn't support Polars styling yet) attempts_pd = attempted_interviews.to_pandas() - # Dynamically set pd styler max elements based on DataFrame size - pd.set_option("styler.render.max_elements", attempts_pd.size + 1) + # Make sure the Styler limit fits this DataFrame + ensure_styler_limit(attempts_pd.size + 1) cmap = sns.light_palette("pink", as_cmap=True) vmin = attempts_pd["num_interviews"].min() diff --git a/src/datasure/checks/summary.py b/src/datasure/checks/summary.py index 97d1c1c8..da52770b 100644 --- a/src/datasure/checks/summary.py +++ b/src/datasure/checks/summary.py @@ -30,6 +30,7 @@ save_check_settings, trigger_save, ) +from datasure.utils.ui_utils import ensure_styler_limit TAB_NAME: str = "summary" @@ -1312,7 +1313,7 @@ def _render_progress_by_column( # Display heatmap cmap = sns.light_palette("pink", as_cmap=True) styler_limit = progress_data.shape[0] * progress_data.shape[1] - pd.set_option("styler.render.max_elements", styler_limit) + ensure_styler_limit(styler_limit) st.dataframe( progress_data.style.format(subset=format_cols, precision=0).background_gradient( subset=format_cols, cmap=cmap, axis=1, vmin=vmin_val, vmax=vmax_val diff --git a/src/datasure/utils/ui_utils.py b/src/datasure/utils/ui_utils.py index cbdd5ce2..bdcbfcac 100644 --- a/src/datasure/utils/ui_utils.py +++ b/src/datasure/utils/ui_utils.py @@ -12,6 +12,7 @@ swap regardless of the order in which the module was first imported. """ +import threading from collections.abc import Callable, Sequence from dataclasses import dataclass from typing import TYPE_CHECKING, Any, Literal @@ -204,22 +205,36 @@ def show_queued_notices(scope: str) -> bool: return bool(notices) -def styled_dataframe(styler: "Styler", **dataframe_kwargs: Any) -> Any: - """Render a pandas ``Styler`` with ``st.dataframe``, whatever its size. +# Serializes updates to pandas' process-wide Styler limit across sessions. +_STYLER_LIMIT_LOCK = threading.Lock() + - Streamlit refuses to render a Styler with more cells than the global - ``styler.render.max_elements`` pandas option, which other pages lower to - fit their own tables. The limit is raised to fit this table only for the - duration of the call, then restored. +def ensure_styler_limit(cells: int) -> None: + """Raise pandas' ``styler.render.max_elements`` to at least `cells`. - Returns what ``st.dataframe`` returns. + Streamlit refuses to render a Styler with more cells than this + process-wide option, and every session shares it. The limit is only + ever raised, never lowered or restored, so one session can't cut it + below what another's render needs. Use this instead of + ``pd.set_option("styler.render.max_elements", ...)``. """ import pandas as pd + + with _STYLER_LIMIT_LOCK: + if pd.get_option("styler.render.max_elements") < cells: + pd.set_option("styler.render.max_elements", cells) + + +def styled_dataframe(styler: "Styler", **dataframe_kwargs: Any) -> Any: + """Render a pandas ``Styler`` with ``st.dataframe``, whatever its size. + + Raises the Styler cell limit to fit the table first (see + `ensure_styler_limit`). Returns what ``st.dataframe`` returns. + """ import streamlit as st - limit = max(pd.get_option("styler.render.max_elements"), styler.data.size) - with pd.option_context("styler.render.max_elements", limit): - return st.dataframe(styler, **dataframe_kwargs) + ensure_styler_limit(styler.data.size) + return st.dataframe(styler, **dataframe_kwargs) def row_styler(df: "pl.DataFrame", row_style: Callable[[Any], list[str]]) -> "Styler": diff --git a/tests/checks/outliers/test_report_ui_corrections.py b/tests/checks/outliers/test_report_ui_corrections.py index c690daa6..146631c1 100644 --- a/tests/checks/outliers/test_report_ui_corrections.py +++ b/tests/checks/outliers/test_report_ui_corrections.py @@ -61,20 +61,22 @@ def violations() -> pl.DataFrame: ) -def _acceptances(*rows: tuple[str, str, str]) -> pl.DataFrame: - """Active acceptances with (KEY, column, reason) rows.""" +def _acceptances(*rows: tuple[str, ...]) -> pl.DataFrame: + """Active acceptances with (KEY, column, reason[, severity]) rows.""" return pl.DataFrame( { "KEY": [r[0] for r in rows], "action": ["accept"] * len(rows), "column": [r[1] for r in rows], "reason": [r[2] for r in rows], + "severity": [r[3] if len(r) > 3 else None for r in rows], }, schema={ "KEY": pl.String, "action": pl.String, "column": pl.String, "reason": pl.String, + "severity": pl.String, }, ) @@ -196,6 +198,29 @@ def test_first_column_is_a_review_button_on_every_flag( assert button_kwargs["key"] == "constraints_flag_review_click" assert button_kwargs["pinned"] is True + def test_button_column_does_not_clash_with_a_survey_column( + self, violations, settings + ): + """A survey field named like the button column can be shown too.""" + data = pl.DataFrame( + { + "KEY": ["K1", "K2", "K3"], + "hhid": ["H1", "H2", "H3"], + "_review": [1, 2, 3], + } + ) + st_mock = _st_mock(clicked=("constraints", 0)) + st_mock.multiselect.return_value = ["_review"] + + dialog = self._render(data, violations, settings, st_mock, _review()) + + shown = st_mock.dataframe.call_args.args[0] + (button_col,) = st_mock.dataframe.call_args.kwargs["column_config"] + assert button_col != REVIEW_BUTTON_COL + assert shown.columns[0] == button_col + assert shown["_review"].to_list() == [1, 2] + assert dialog.call_args.args[2].key_value == "K1" + def test_rows_are_not_selectable(self, data, violations, settings): st_mock = _st_mock() @@ -239,7 +264,9 @@ def test_a_click_on_the_other_table_opens_no_dialog( def test_accepted_violations_are_hidden(self, data, violations, settings): st_mock = _st_mock() - review = _review({"constraints": _acceptances(("K1", "age", "verified"))}) + review = _review( + {"constraints": _acceptances(("K1", "age", "verified", "hard"))} + ) self._render(data, violations, settings, st_mock, review) @@ -249,7 +276,9 @@ def test_show_reviewed_shows_them_with_badge_and_reason( self, data, violations, settings ): st_mock = _st_mock(show_reviewed=True) - review = _review({"constraints": _acceptances(("K1", "age", "verified"))}) + review = _review( + {"constraints": _acceptances(("K1", "age", "verified", "hard"))} + ) self._render(data, violations, settings, st_mock, review) @@ -263,7 +292,9 @@ def test_show_reviewed_colours_reviewed_rows_green( self, data, violations, settings ): st_mock = _st_mock(show_reviewed=True) - review = _review({"constraints": _acceptances(("K1", "age", "verified"))}) + review = _review( + {"constraints": _acceptances(("K1", "age", "verified", "hard"))} + ) self._render(data, violations, settings, st_mock, review) @@ -302,7 +333,7 @@ def test_show_only_reviewed_lists_accepted_and_corrected_rows( ): st_mock = _st_mock(reviewed_only=True) review = _review( - {"constraints": _acceptances(("K1", "age", "verified"))}, + {"constraints": _acceptances(("K1", "age", "verified", "hard"))}, corrections=_acceptances(("K3", "age", "typo fixed")), ) @@ -346,12 +377,25 @@ def test_without_show_reviewed_the_table_is_not_styled( self, data, violations, settings ): st_mock = _st_mock() - review = _review({"constraints": _acceptances(("K1", "age", "verified"))}) + review = _review( + {"constraints": _acceptances(("K1", "age", "verified", "hard"))} + ) self._render(data, violations, settings, st_mock, review) assert not isinstance(st_mock.dataframe.call_args.args[0], Styler) + def test_soft_acceptance_does_not_hide_a_hard_violation( + self, data, violations, settings + ): + """K1 was accepted as a soft violation; bounds now make it hard.""" + st_mock = _st_mock() + review = _review({"constraints": _acceptances(("K1", "age", "verified"))}) + + self._render(data, violations, settings, st_mock, review) + + assert _shown_table(st_mock)["KEY"].to_list() == ["K1", "K2"] + def test_outlier_acceptance_does_not_hide_a_constraint_violation( self, data, violations, settings ): @@ -759,7 +803,7 @@ def test_constraint_metrics_do_not_count_accepted_violations( data, violations, outliers, - {"constraints": _acceptances(("K1", "age", "verified"))}, + {"constraints": _acceptances(("K1", "age", "verified", "hard"))}, ) counted = metrics.call_args.args[0] diff --git a/tests/checks/outliers/test_review.py b/tests/checks/outliers/test_review.py index 74823d4b..90de9d00 100644 --- a/tests/checks/outliers/test_review.py +++ b/tests/checks/outliers/test_review.py @@ -40,12 +40,21 @@ def _acceptances(rows: list[dict]) -> pl.DataFrame: "current_value": r.get("current_value"), "reason": r.get("reason", "checked"), "check_type": r.get("check_type", "outliers"), + "severity": r.get("severity"), } for r in rows ] ).select( pl.col(c).cast(pl.String) - for c in ["KEY", "action", "column", "current_value", "reason", "check_type"] + for c in [ + "KEY", + "action", + "column", + "current_value", + "reason", + "check_type", + "severity", + ] ) @@ -131,7 +140,14 @@ def test_matches_non_string_keys_as_text(self): } ) acceptances = _acceptances( - [{"KEY": "1", "column": "age", "check_type": "constraints"}] + [ + { + "KEY": "1", + "column": "age", + "check_type": "constraints", + "severity": "hard", + } + ] ) result = mark_reviewed(flags, acceptances, "survey_key", CONSTRAINTS) @@ -512,3 +528,62 @@ def test_reviewed_only_without_review_columns_shows_nothing(self, outlier_flags) assert result.is_empty() assert result.columns == outlier_flags.columns + + +class TestHardViolationsNeedHardAcceptances: + """A soft acceptance must not silence a value that is now a hard violation.""" + + @pytest.fixture + def violations(self) -> pl.DataFrame: + # Bounds were tightened: age 70 is now above the hard maximum. + return pl.DataFrame( + { + "survey_key": ["K1", "K2"], + "column name": ["age", "age"], + "violation reason": [ + "Value is above hard maximum 60.0", + "Value is above soft maximum 50.0", + ], + } + ) + + def _mark(self, violations, severity): + acceptances = _acceptances( + [ + {"KEY": key, "column": "age", "check_type": "constraints", **severity} + for key in ("K1", "K2") + ] + ) + return mark_reviewed(violations, acceptances, "survey_key", CONSTRAINTS) + + def test_soft_acceptance_leaves_a_hard_violation_pending(self, violations): + marked = self._mark(violations, {}) + + assert marked[REVIEW_STATUS_COL].to_list() == [None, REVIEWED_BADGE] + + def test_hard_acceptance_covers_a_hard_violation(self, violations): + marked = self._mark(violations, {"severity": "hard"}) + + assert marked[REVIEW_STATUS_COL].to_list() == [REVIEWED_BADGE, REVIEWED_BADGE] + + def test_pending_hard_violation_is_counted_and_offers_confirmed_accept( + self, violations + ): + marked = self._mark(violations, {}) + table = marked.with_columns( + pl.Series("violation type", ["Hard Max", "Soft Max"]) + ) + + counted = clear_reviewed_flags(marked, CONSTRAINTS) + selection = select_flag(table, [0], "survey_key", CONSTRAINTS) + + assert counted["violation reason"][0] == "Value is above hard maximum 60.0" + assert Action.ACCEPT in allowed_actions(selection) + assert needs_hard_confirmation(selection, Action.ACCEPT) + + def test_acceptances_without_a_severity_column_still_work(self, outlier_flags): + acceptances = _acceptances([{"KEY": "K1", "column": "age"}]).drop("severity") + + marked = mark_reviewed(outlier_flags, acceptances, "survey_key", OUTLIERS) + + assert marked[REVIEW_STATUS_COL][0] == REVIEWED_BADGE diff --git a/tests/utils/test_ui_utils.py b/tests/utils/test_ui_utils.py index 4e10fc05..9c5f5923 100644 --- a/tests/utils/test_ui_utils.py +++ b/tests/utils/test_ui_utils.py @@ -1,6 +1,7 @@ """Tests for the shared UI helpers module.""" import sys +import threading from unittest.mock import MagicMock import pandas as pd @@ -9,6 +10,7 @@ from datasure.utils.ui_utils import ( confirm_dialog, + ensure_styler_limit, metric_row, page_header, queue_notice, @@ -242,16 +244,44 @@ def test_toast_notices_render_as_toasts(self, st_with_state): st_with_state.toast.assert_called_once_with("Saved") -class TestStyledDataframe: - """Styled tables render whatever their size, without leaking the limit.""" +@pytest.fixture +def small_limit(): + """Start from a low global Styler limit and restore it afterwards.""" + with pd.option_context("styler.render.max_elements", 10): + yield - @pytest.fixture - def small_limit(self): - """Simulate another page having lowered the global Styler limit.""" - with pd.option_context("styler.render.max_elements", 10): - yield - def test_raises_the_styler_limit_to_fit_the_table(self, mock_st, small_limit): +class TestEnsureStylerLimit: + """The global Styler limit only ever goes up.""" + + def test_raises_a_lower_limit(self, small_limit): + ensure_styler_limit(100) + + assert pd.get_option("styler.render.max_elements") == 100 + + def test_never_lowers_the_limit(self, small_limit): + ensure_styler_limit(100) + ensure_styler_limit(20) + + assert pd.get_option("styler.render.max_elements") == 100 + + def test_concurrent_raises_keep_the_largest(self, small_limit): + sizes = list(range(11, 211)) + threads = [ + threading.Thread(target=ensure_styler_limit, args=(size,)) for size in sizes + ] + for thread in threads: + thread.start() + for thread in threads: + thread.join() + + assert pd.get_option("styler.render.max_elements") == max(sizes) + + +class TestStyledDataframe: + """Styled tables render whatever their size.""" + + def test_the_limit_fits_the_table_while_it_renders(self, mock_st, small_limit): styler = pd.DataFrame({"a": range(50), "b": range(50)}).style seen_limits = [] mock_st.dataframe.side_effect = lambda *a, **k: seen_limits.append( @@ -263,10 +293,11 @@ def test_raises_the_styler_limit_to_fit_the_table(self, mock_st, small_limit): assert seen_limits == [100] mock_st.dataframe.assert_called_once_with(styler, width="stretch") - def test_restores_the_previous_limit(self, mock_st, small_limit): + def test_the_limit_is_not_lowered_afterwards(self, mock_st, small_limit): + """Restoring a lower limit could break a render in another session.""" styled_dataframe(pd.DataFrame({"a": range(50)}).style) - assert pd.get_option("styler.render.max_elements") == 10 + assert pd.get_option("styler.render.max_elements") == 50 def test_never_lowers_a_higher_limit(self, mock_st): seen_limits = [] From 9215b9294b73a2e61e904976048a96bfcad3bfd2 Mon Sep 17 00:00:00 2001 From: iabaako Date: Fri, 2 Oct 2026 07:53:01 +0000 Subject: [PATCH 11/12] fix(outliers): address third Copilot review of check-page corrections - Sort results tables by KEY, column name and flag reason so a Review click resolves to the same flag on the rerun it triggers, whatever order the join returns. - Keep the computed flag columns authoritative when joining survey display columns: rename clashing survey fields (and "violation type") with a " (survey)" suffix instead of dropping the flag's column, so a survey field named "column name" can no longer become the correction target. Co-Authored-By: Claude Opus 5.5 --- src/datasure/checks/outliers/report_ui.py | 30 ++++----- src/datasure/checks/outliers/review.py | 54 +++++++++++++++- tests/checks/outliers/test_review.py | 76 +++++++++++++++++++++++ 3 files changed, 139 insertions(+), 21 deletions(-) diff --git a/src/datasure/checks/outliers/report_ui.py b/src/datasure/checks/outliers/report_ui.py index 5057b251..d707af99 100644 --- a/src/datasure/checks/outliers/report_ui.py +++ b/src/datasure/checks/outliers/report_ui.py @@ -36,6 +36,7 @@ CONSTRAINTS, OUTLIERS, REVIEW_STATUS_COL, + VIOLATION_TYPE_COL, FlagCheck, FlagSelection, TableFilters, @@ -43,6 +44,7 @@ clear_reviewed_flags, filter_table, highlight_reviewed_row, + join_survey_columns, mark_reviewed, needs_hard_confirmation, select_flag, @@ -56,7 +58,7 @@ render_correction_inputs, should_enable_apply_button, ) -from datasure.utils.dataframe_utils import ColumnByType, sanitize_df_for_join +from datasure.utils.dataframe_utils import ColumnByType from datasure.utils.duckdb_utils import duckdb_get_table, duckdb_save_table from datasure.utils.navigations_utils import demo_callout from datasure.utils.onboarding_utils import is_demo_project @@ -550,17 +552,12 @@ def _render_constraint_violations_table( # select columns to display from data display_df = data.select(include_cols) - # sanitize violation_data to avoid column name conflicts - violation_df = sanitize_df_for_join( - main_df=display_df, - join_df=violation_data, - join_key=settings.survey_key, - ) - - violations_df = display_df.join( - violation_df, - on=settings.survey_key, - how="inner", + violations_df = join_survey_columns( + display_df, + violation_data, + settings.survey_key, + CONSTRAINTS, + reserved=[VIOLATION_TYPE_COL], ) # add violation type column ie. "Soft Min", "Soft Max", "Hard Min", "Hard Max" @@ -580,7 +577,7 @@ def _render_constraint_violations_table( ) violations_df = violations_df.with_columns( - violation_type_expr.alias("violation type") + violation_type_expr.alias(VIOLATION_TYPE_COL) ) _render_flags_table(violations_df, data, settings, CONSTRAINTS, review) @@ -709,11 +706,8 @@ def _render_outlier_column_inspection( # select columns to display from data display_df = data.select(include_cols) - outliers_df = sanitize_df_for_join(display_df, outliers_data, settings.survey_key) - display_df = display_df.join( - outliers_df, - on=settings.survey_key, - how="inner", + display_df = join_survey_columns( + display_df, outliers_data, settings.survey_key, OUTLIERS ) _render_flags_table( diff --git a/src/datasure/checks/outliers/review.py b/src/datasure/checks/outliers/review.py index 79c13a40..a19e6a62 100644 --- a/src/datasure/checks/outliers/review.py +++ b/src/datasure/checks/outliers/review.py @@ -15,6 +15,7 @@ Kept free of Streamlit so the logic can be tested without a running app. """ +from collections.abc import Sequence from dataclasses import dataclass from typing import Any @@ -40,6 +41,11 @@ class FlagCheck: OUTLIERS = FlagCheck("outliers", "outlier reason", "no outlier") CONSTRAINTS = FlagCheck("constraints", "violation reason", "no violation") +# Flag columns of the computed results that the review logic reads. +COLUMN_NAME_COL = "column name" +# Added to the constraint table by `_render_constraint_violations_table`. +VIOLATION_TYPE_COL = "violation type" + # Violation types (see `_render_constraint_violations_table`) of hard bounds. _HARD_VIOLATION_TYPES = ("Hard Min", "Hard Max") @@ -135,7 +141,7 @@ def mark_reviewed( keyed = flags.with_columns( pl.col(survey_key).cast(pl.String).alias("_review_key"), - pl.col("column name").cast(pl.String).alias("_review_column"), + pl.col(COLUMN_NAME_COL).cast(pl.String).alias("_review_column"), ) for prefix, log_rows in (("accept", acceptances), ("correct", corrections)): keyed = keyed.join( @@ -271,6 +277,48 @@ def filter_table( return flags +SURVEY_COL_SUFFIX = " (survey)" + + +def join_survey_columns( + survey: pl.DataFrame, + flags: pl.DataFrame, + survey_key: str, + check: FlagCheck, + *, + reserved: Sequence[str] = (), +) -> pl.DataFrame: + """Join survey display columns onto `flags` for a results table. + + The flag columns stay authoritative: a survey column named like one of + them, or like a `reserved` column added afterwards, is renamed with + `SURVEY_COL_SUFFIX`. Otherwise a survey field called "column name" + would become the correction target. + + The result is sorted by KEY, column name and flag reason, so a row + position reported by a Review click resolves to the same flag on the + rerun it triggers whatever order the join returns. + """ + taken = set(flags.columns) | set(reserved) | set(survey.columns) + renames = {} + for col in survey.columns: + if col == survey_key or (col not in flags.columns and col not in reserved): + continue + new_name = f"{col}{SURVEY_COL_SUFFIX}" + while new_name in taken: + new_name = f"{new_name}{SURVEY_COL_SUFFIX}" + renames[col] = new_name + taken.add(new_name) + + joined = survey.rename(renames).join(flags, on=survey_key, how="inner") + sort_cols = [ + col + for col in (survey_key, COLUMN_NAME_COL, check.reason_col) + if col in joined.columns + ] + return joined.sort(sort_cols, nulls_last=True, maintain_order=True) + + def select_flag( table: pl.DataFrame, rows: list[int], @@ -305,11 +353,11 @@ def select_flag( ) return FlagSelection( key_value=row[survey_key], - column=row["column name"], + column=row[COLUMN_NAME_COL], check_type=check.check_type, flagged=flagged, reviewed=row.get(REVIEW_STATUS_COL) == REVIEWED_BADGE, - hard=row.get("violation type") in _HARD_VIOLATION_TYPES, + hard=row.get(VIOLATION_TYPE_COL) in _HARD_VIOLATION_TYPES, ) diff --git a/tests/checks/outliers/test_review.py b/tests/checks/outliers/test_review.py index 90de9d00..e0d1c667 100644 --- a/tests/checks/outliers/test_review.py +++ b/tests/checks/outliers/test_review.py @@ -11,6 +11,8 @@ REVIEW_REASON_COL, REVIEW_STATUS_COL, REVIEWED_BADGE, + SURVEY_COL_SUFFIX, + VIOLATION_TYPE_COL, FlagSelection, TableFilters, allowed_actions, @@ -18,6 +20,7 @@ filter_table, flagged_only, highlight_reviewed_row, + join_survey_columns, mark_reviewed, needs_hard_confirmation, select_flag, @@ -300,6 +303,79 @@ def constraint_table() -> pl.DataFrame: ) +class TestJoinSurveyColumns: + def test_flag_column_name_wins_over_a_survey_field_of_that_name( + self, outlier_flags + ): + survey = pl.DataFrame( + {"survey_key": ["K1", "K2", "K3"], "column name": ["income"] * 3} + ) + + table = join_survey_columns(survey, outlier_flags, "survey_key", OUTLIERS) + selection = select_flag(table, [0], "survey_key", OUTLIERS) + + assert selection.column == "age" + assert table[f"column name{SURVEY_COL_SUFFIX}"].to_list() == ["income"] * 4 + + def test_reserved_names_are_renamed_on_the_survey_side(self, outlier_flags): + survey = pl.DataFrame( + { + "survey_key": ["K1", "K2", "K3"], + VIOLATION_TYPE_COL: ["a", "b", "c"], + f"{VIOLATION_TYPE_COL}{SURVEY_COL_SUFFIX}": ["x", "y", "z"], + } + ) + + table = join_survey_columns( + survey, + outlier_flags, + "survey_key", + OUTLIERS, + reserved=[VIOLATION_TYPE_COL], + ) + + assert VIOLATION_TYPE_COL not in table.columns + assert f"{VIOLATION_TYPE_COL}{SURVEY_COL_SUFFIX * 2}" in table.columns + assert f"{VIOLATION_TYPE_COL}{SURVEY_COL_SUFFIX}" in table.columns + + def test_row_order_does_not_depend_on_input_order(self, outlier_flags): + survey = pl.DataFrame( + {"survey_key": ["K1", "K2", "K3"], "enumerator": ["E1", "E2", "E3"]} + ) + + table = join_survey_columns(survey, outlier_flags, "survey_key", OUTLIERS) + reordered = join_survey_columns( + survey.reverse(), outlier_flags.reverse(), "survey_key", OUTLIERS + ) + + assert table.equals(reordered) + # A click on row 1 of the first render resolves to the same flag after + # a rerun whose join returned rows in another order. + assert select_flag(table, [1], "survey_key", OUTLIERS) == select_flag( + reordered, [1], "survey_key", OUTLIERS + ) + + def test_duplicate_key_and_column_are_ordered_by_reason(self): + flags = pl.DataFrame( + { + "survey_key": ["K1", "K1"], + "column name": ["age", "age"], + "violation reason": [ + "Value is above soft maximum 65", + "Value is above hard maximum 100", + ], + } + ) + survey = pl.DataFrame({"survey_key": ["K1"]}) + + table = join_survey_columns(survey, flags, "survey_key", CONSTRAINTS) + + assert table["violation reason"].to_list() == [ + "Value is above hard maximum 100", + "Value is above soft maximum 65", + ] + + class TestSelectFlag: def test_prefills_key_and_column_from_the_selected_row(self, constraint_table): selection = select_flag(constraint_table, [1], "survey_key", CONSTRAINTS) From 77b39bf3bd2ad42fd02a3e73b8edbcb7907dab5d Mon Sep 17 00:00:00 2001 From: iabaako Date: Sat, 3 Oct 2026 19:20:11 +0100 Subject: [PATCH 12/12] fix(outliers): address fourth Copilot review of check-page corrections - Review metadata can't clash with survey names: mark_reviewed matches the log in a frame of its own helper columns, so a KEY named like a helper (e.g. "_review_key") is no longer replaced or dropped. join_survey_columns always renames survey fields named "review status" or "review reason", even while those columns are hidden, so survey text is never read as review state. A Survey KEY named like a review column turns review off with a warning instead of being overwritten - Duplicate KEYs: Review on a KEY whose rows hold different values of the column shows a warning instead of the form, since a correction changes every row with the KEY (review.key_has_conflicting_values) - Failed corrections: get_active_corrections only returns corrections with status "Successful", so one that failed to reapply is never shown as Corrected - Acceptance severity: apply_corrections rejects a severity on anything but a constraint acceptance, and any severity other than "hard" Refs #298 Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 13 ++- docs/USER_GUIDE.md | 6 ++ src/datasure/checks/outliers/report_ui.py | 26 ++++- src/datasure/checks/outliers/review.py | 68 ++++++++---- src/datasure/processing/corrections.py | 29 ++++- .../outliers/test_report_ui_corrections.py | 91 +++++++++++++++- tests/checks/outliers/test_review.py | 100 ++++++++++++++++++ tests/processing/test_corrections.py | 47 ++++++++ 8 files changed, 350 insertions(+), 30 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c1babba0..1263af8a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -68,10 +68,19 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 after raising the limit to fit it; the results tables and the Correction Log use it. A soft-violation acceptance no longer hides a value that has since become a hard violation (e.g. after bounds are tightened): it needs a - new hard acceptance — #298 + new hard acceptance. A correction that failed to reapply to new prep data + is never shown as Corrected, even if the data holds its new value. Review + on a KEY whose rows hold different values of the column shows a warning + instead of the form, since a correction changes every row with the KEY + (`review.key_has_conflicting_values`). Survey fields added through "Show + more columns" that share a name with a results or review column get a + " (survey)" suffix, even while the review columns are hidden. A Survey KEY + named "review status" or "review reason" turns review off with a warning + instead of being overwritten — #298 - **Correction log severity**: New `severity` column, `hard` on acceptances of hard constraint violations (null otherwise and for legacy logs). - `CorrectionEntry.severity` sets it and is rejected on non-accept actions. + `CorrectionEntry.severity` sets it and is rejected on non-accept actions, + on acceptances of other checks, and with any value other than `hard`. Hard acceptances are highlighted in the Correction Log — #298 ### Fixed diff --git a/docs/USER_GUIDE.md b/docs/USER_GUIDE.md index a4ab6dc9..0ffff2fe 100644 --- a/docs/USER_GUIDE.md +++ b/docs/USER_GUIDE.md @@ -862,6 +862,12 @@ if the value changes, or if you remove the acceptance on the Correct Data page. Every entry appears in the Correction Log with source `outliers` or `constraints`. +Corrections and acceptances apply to every row with the same KEY. If a KEY is +on more than one row with different values in the flagged column, Review shows +a warning instead of the form, because a correction would change all those +rows. Give each record a unique KEY in the source data first. The Duplicates +check lists duplicated KEYs. + --- ### 6. Enumerator Stats Report diff --git a/src/datasure/checks/outliers/report_ui.py b/src/datasure/checks/outliers/report_ui.py index d707af99..bfc9973a 100644 --- a/src/datasure/checks/outliers/report_ui.py +++ b/src/datasure/checks/outliers/report_ui.py @@ -35,6 +35,7 @@ from datasure.checks.outliers.review import ( CONSTRAINTS, OUTLIERS, + REVIEW_COLUMNS, REVIEW_STATUS_COL, VIOLATION_TYPE_COL, FlagCheck, @@ -45,6 +46,7 @@ filter_table, highlight_reviewed_row, join_survey_columns, + key_has_conflicting_values, mark_reviewed, needs_hard_confirmation, select_flag, @@ -341,9 +343,24 @@ def _render_flag_correction_form( the data. Accepting a hard constraint violation needs an extra confirmation and is logged with severity "hard". A successful save reruns the page so the tables and metrics reflect it. + + A KEY shared by rows with different values of the column can't be + reviewed, since a correction would change every one of those rows. """ key_col = settings.survey_key key_value = selection.key_value + + st.markdown(f"**{selection.column}** for KEY **{key_value}**") + if key_has_conflicting_values(data, key_col, key_value, selection.column): + st.warning( + f"KEY {key_value} is on more than one row, with different values " + f"of {selection.column}. Corrections and acceptances apply to " + "every row with the KEY, so this value can't be reviewed here. " + "Give each record a unique KEY in the source data first; the " + "Duplicates check lists duplicated KEYs." + ) + return + current_value = get_current_value(data, key_col, key_value, selection.column) survey_id_value = ( get_current_value(data, key_col, key_value, settings.survey_id) @@ -354,7 +371,6 @@ def _render_flag_correction_form( # "1_b" would collide if joined with underscores. namespace = json.dumps([selection.check_type, str(key_value), selection.column]) - st.markdown(f"**{selection.column}** for KEY **{key_value}**") if selection.reviewed: st.info( "This flag was accepted as valid. Remove the acceptance on the " @@ -1508,6 +1524,14 @@ def outliers_report( outliers_settings = outliers_report_settings( setting_file, config_settings, categorical_columns, datetime_columns ) + if review is not None and outliers_settings.survey_key in REVIEW_COLUMNS: + st.warning( + f"Flags can't be corrected or accepted on this page because the " + f"Survey KEY column is named '{outliers_settings.survey_key}', " + "which this report uses for review results. Rename the column to " + "review flags here." + ) + review = None # Outlier columns configuration st.subheader("Outlier/Constraint Columns Configuration") diff --git a/src/datasure/checks/outliers/review.py b/src/datasure/checks/outliers/review.py index a19e6a62..e08b5443 100644 --- a/src/datasure/checks/outliers/review.py +++ b/src/datasure/checks/outliers/review.py @@ -25,6 +25,8 @@ REVIEW_STATUS_COL = "review status" REVIEW_REASON_COL = "review reason" +# The columns `mark_reviewed` adds to the results. +REVIEW_COLUMNS = (REVIEW_STATUS_COL, REVIEW_REASON_COL) REVIEWED_BADGE = "Reviewed" CORRECTED_BADGE = "Corrected" @@ -80,11 +82,11 @@ def _latest_entry_by_cell(log_rows: pl.DataFrame, prefix: str) -> pl.DataFrame: else pl.lit(None, dtype=pl.String) ) return log_rows.select( - pl.col("KEY").cast(pl.String).alias("_review_key"), - pl.col("column").cast(pl.String).alias("_review_column"), + pl.col("KEY").cast(pl.String).alias("_key"), + pl.col("column").cast(pl.String).alias("_column"), pl.col("reason").cast(pl.String).alias(f"_{prefix}_reason"), severity.alias(f"_{prefix}_severity"), - ).unique(subset=["_review_key", "_review_column"], keep="last") + ).unique(subset=["_key", "_column"], keep="last") def _is_hard_violation(check: FlagCheck) -> pl.Expr: @@ -132,21 +134,33 @@ def mark_reviewed( an accepted flag (only flagged rows can be accepted), `CORRECTED_BADGE` and the correction reason for a corrected cell, otherwise null. An acceptance takes precedence over a correction. + + Raises + ------ + ValueError + If `survey_key` is one of `REVIEW_COLUMNS`, which would overwrite it. """ if flags.is_empty(): return flags + if survey_key in REVIEW_COLUMNS: + raise ValueError( + f"The Survey KEY column '{survey_key}' has the name of a review column" + ) if corrections is None: corrections = acceptances.clear() - keyed = flags.with_columns( - pl.col(survey_key).cast(pl.String).alias("_review_key"), - pl.col(COLUMN_NAME_COL).cast(pl.String).alias("_review_column"), + # Match in a frame of our own columns, so the helper columns can't clash + # with a survey KEY or field of the same name. + cells = flags.select( + pl.col(survey_key).cast(pl.String).alias("_key"), + pl.col(COLUMN_NAME_COL).cast(pl.String).alias("_column"), + pl.col(check.reason_col), ) for prefix, log_rows in (("accept", acceptances), ("correct", corrections)): - keyed = keyed.join( + cells = cells.join( _latest_entry_by_cell(log_rows, prefix), - on=["_review_key", "_review_column"], + on=["_key", "_column"], how="left", maintain_order="left", ) @@ -163,7 +177,8 @@ def mark_reviewed( ) ) corrected = pl.col("_correct_reason").is_not_null() - return keyed.with_columns( + # Each cell matches at most one log row, so `cells` lines up with `flags`. + review = cells.select( pl.when(accepted) .then(pl.lit(REVIEWED_BADGE)) .when(corrected) @@ -174,14 +189,8 @@ def mark_reviewed( .when(corrected) .then(pl.col("_correct_reason")) .alias(REVIEW_REASON_COL), - ).drop( - "_review_key", - "_review_column", - "_accept_reason", - "_accept_severity", - "_correct_reason", - "_correct_severity", ) + return flags.with_columns(review.get_columns()) def _is_reviewed() -> pl.Expr: @@ -291,18 +300,21 @@ def join_survey_columns( """Join survey display columns onto `flags` for a results table. The flag columns stay authoritative: a survey column named like one of - them, or like a `reserved` column added afterwards, is renamed with + them, like a review column (`REVIEW_COLUMNS`, even when hidden), or like + a `reserved` column added afterwards, is renamed with `SURVEY_COL_SUFFIX`. Otherwise a survey field called "column name" - would become the correction target. + would become the correction target, and one called "review status" + would be read as the flag's review state. The result is sorted by KEY, column name and flag reason, so a row position reported by a Review click resolves to the same flag on the rerun it triggers whatever order the join returns. """ - taken = set(flags.columns) | set(reserved) | set(survey.columns) + generated = set(flags.columns) | set(REVIEW_COLUMNS) | set(reserved) + taken = generated | set(survey.columns) renames = {} for col in survey.columns: - if col == survey_key or (col not in flags.columns and col not in reserved): + if col == survey_key or col not in generated: continue new_name = f"{col}{SURVEY_COL_SUFFIX}" while new_name in taken: @@ -361,6 +373,22 @@ def select_flag( ) +def key_has_conflicting_values( + data: pl.DataFrame, survey_key: str, key_value: Any, column: str +) -> bool: + """Whether rows sharing `key_value` hold different values of `column`. + + Corrections and acceptances target a KEY, not a row: a modification + changes every row with the KEY, and an acceptance holds only while every + row has the accepted value. A flag on such a KEY can't be reviewed + without changing or misreading another record. + """ + if survey_key not in data.columns or column not in data.columns: + return False + values = data.filter(pl.col(survey_key) == key_value).get_column(column) + return values.n_unique() > 1 + + def allowed_actions(selection: FlagSelection) -> list[Action]: """Return the actions the correction form offers for `selection`. diff --git a/src/datasure/processing/corrections.py b/src/datasure/processing/corrections.py index 9ca1e7e5..27447d48 100644 --- a/src/datasure/processing/corrections.py +++ b/src/datasure/processing/corrections.py @@ -13,6 +13,7 @@ CORRECTION_ACTIONS, CORRECTION_LOG_SCHEMA, CORRECTIONS_PAGE_SOURCE, + HARD_SEVERITY, Action, empty_correction_log, ensure_log_columns, @@ -179,14 +180,28 @@ def _check_acceptance_against_data( def _validate_acceptance( - check_type: str | None, column: str | None, current_value: Any + check_type: str | None, + column: str | None, + current_value: Any, + severity: str | None = None, ) -> None: - """Raise ValueError if an acceptance's check type, column and value don't fit.""" + """Raise ValueError if an acceptance's check type, column, value and + severity don't fit. + """ if check_type not in ACCEPT_CHECK_TYPES: raise ValueError( f"Unknown check type '{check_type}'; expected one of " f"{', '.join(ACCEPT_CHECK_TYPES)}" ) + # The Correction Log highlights hard acceptances as hard-constraint + # overrides, so only a constraint acceptance may be hard. + if severity is not None and ( + severity != HARD_SEVERITY or check_type != "constraints" + ): + raise ValueError( + f"Severity '{severity}' is not valid for a {check_type} acceptance; " + f"only constraint acceptances can have severity '{HARD_SEVERITY}'" + ) if check_type == "gps": if ( column is not None @@ -576,7 +591,8 @@ def get_active_corrections(self, alias: str, key_col: str) -> pl.DataFrame: A "modify value" is active while the cell holds its new value, and a "remove value" while the cell is missing. A correction overwritten by - a later one, or whose row was removed, is inactive. + a later one, whose row was removed, or that failed to reapply to the + current prep data, is inactive. Parameters ---------- @@ -595,9 +611,12 @@ def get_active_corrections(self, alias: str, key_col: str) -> pl.DataFrame: if log.width == 0: return empty_correction_log() + # A correction that failed to reapply was not applied, even if the + # prep data happens to hold its new value. corrections = log.filter( pl.col("action").is_in([Action.MODIFY_VALUE, Action.REMOVE_VALUE]) & pl.col("column").is_not_null() + & (pl.col("status") == "Successful") ) if corrections.is_empty(): return corrections @@ -770,7 +789,9 @@ def _apply_entry( ) if entry.action == Action.ACCEPT: - _validate_acceptance(entry.check_type, entry.column, entry.current_value) + _validate_acceptance( + entry.check_type, entry.column, entry.current_value, entry.severity + ) _check_acceptance_against_data( data, key_col, entry.key_value, entry.column, entry.current_value ) diff --git a/tests/checks/outliers/test_report_ui_corrections.py b/tests/checks/outliers/test_report_ui_corrections.py index 146631c1..8cb62e4e 100644 --- a/tests/checks/outliers/test_report_ui_corrections.py +++ b/tests/checks/outliers/test_report_ui_corrections.py @@ -221,6 +221,30 @@ def test_button_column_does_not_clash_with_a_survey_column( assert shown["_review"].to_list() == [1, 2] assert dialog.call_args.args[2].key_value == "K1" + def test_a_survey_review_status_field_is_not_read_as_review_state( + self, violations, settings + ): + """A survey field named "review status" is shown, renamed, and ignored.""" + data = pl.DataFrame( + { + "KEY": ["K1", "K2", "K3"], + "hhid": ["H1", "H2", "H3"], + "review status": ["Reviewed"] * 3, + } + ) + st_mock = _st_mock(clicked=("constraints", 1)) + st_mock.multiselect.return_value = ["review status"] + + dialog = self._render(data, violations, settings, st_mock, _review()) + + shown = st_mock.dataframe.call_args.args[0] + assert not isinstance(shown, Styler) + assert "review status" not in shown.columns + assert shown["review status (survey)"].to_list() == ["Reviewed"] * 2 + selection = dialog.call_args.args[2] + assert selection.key_value == "K2" + assert not selection.reviewed + def test_rows_are_not_selectable(self, data, violations, settings): st_mock = _st_mock() @@ -723,6 +747,44 @@ def test_widget_keys_are_unique_per_cell(self, data, settings): assert namespaces[0] != namespaces[1] + def test_duplicate_key_with_different_values_cannot_be_reviewed(self, settings): + """Reviewing age 150 must not prefill or overwrite the other K1's 30.""" + duplicated = pl.DataFrame( + {"KEY": ["K1", "K1"], "hhid": ["H1", "H1"], "age": [30, 150]} + ) + + st_mock, inputs, apply_entries = self._render( + duplicated, + settings, + _selection(key_value="K1", hard=True), + _review(), + _form_state(key_value="K1", action=Action.MODIFY_VALUE, new_value="90"), + apply=True, + confirm=True, + ) + + assert "more than one row" in st_mock.warning.call_args.args[0] + inputs.assert_not_called() + apply_entries.assert_not_called() + st_mock.button.assert_not_called() + + def test_duplicate_key_with_the_same_value_can_be_reviewed(self, settings): + duplicated = pl.DataFrame( + {"KEY": ["K1", "K1"], "hhid": ["H1", "H1"], "age": [150, 150]} + ) + + _, inputs, _ = self._render( + duplicated, + settings, + _selection(key_value="K1"), + _review(), + _form_state(key_value="K1"), + apply=False, + confirm=False, + ) + + assert inputs.call_args.kwargs["current_value"] == 150 + def test_failed_save_does_not_rerun(self, data, settings): st_mock = _st_mock() st_mock.button.return_value = True @@ -752,8 +814,10 @@ def outliers(self) -> pl.DataFrame: } ) - def _run_report(self, data, violations, outliers, acceptances_by_check): - config = {"survey_key": "KEY", "survey_id": "hhid"} + def _run_report( + self, data, violations, outliers, acceptances_by_check, survey_key="KEY" + ): + config = {"survey_key": survey_key, "survey_id": "hhid"} columns = ColumnByType( all_columns=data.columns, categorical_columns=[], @@ -763,8 +827,9 @@ def _run_report(self, data, violations, outliers, acceptances_by_check): integer_columns=["age"], ) processor = _review(acceptances_by_check).processor + self.st_mock = _st_mock() with ( - patch(f"{MODULE}.st", _st_mock()), + patch(f"{MODULE}.st", self.st_mock), patch( f"{MODULE}.outliers_report_settings", return_value=OutlierSettings(**config), @@ -836,3 +901,23 @@ def test_corrections_are_looked_up_once_per_report_run( self._run_report(data, violations, outliers, {}) assert self.processor.get_active_corrections.call_count == 1 + + def test_a_key_named_like_a_review_column_turns_review_off( + self, data, violations, outliers + ): + """The report still renders, without overwriting the KEY column.""" + key = "review status" + + _, table, _, inspection = self._run_report( + data.rename({"KEY": key}), + violations.rename({"KEY": key}), + outliers.rename({"KEY": key}), + {"constraints": _acceptances(("K1", "age", "verified", "hard"))}, + survey_key=key, + ) + + assert key in self.st_mock.warning.call_args.args[0] + assert table.call_args.kwargs["review"] is None + assert inspection.call_args.kwargs["review"] is None + assert table.call_args.args[1][key].to_list() == ["K1", "K2", "K3"] + self.processor.get_active_acceptances.assert_not_called() diff --git a/tests/checks/outliers/test_review.py b/tests/checks/outliers/test_review.py index e0d1c667..e1a48a1f 100644 --- a/tests/checks/outliers/test_review.py +++ b/tests/checks/outliers/test_review.py @@ -8,6 +8,7 @@ CONSTRAINTS, CORRECTED_BADGE, OUTLIERS, + REVIEW_COLUMNS, REVIEW_REASON_COL, REVIEW_STATUS_COL, REVIEWED_BADGE, @@ -21,6 +22,7 @@ flagged_only, highlight_reviewed_row, join_survey_columns, + key_has_conflicting_values, mark_reviewed, needs_hard_confirmation, select_flag, @@ -182,6 +184,26 @@ def test_empty_flags_are_returned_unchanged(self): assert result.is_empty() + @pytest.mark.parametrize("key_name", ["_key", "_review_key", "_accept_reason"]) + def test_a_key_named_like_a_helper_column_is_kept(self, outlier_flags, key_name): + flags = outlier_flags.rename({"survey_key": key_name}) + acceptances = _acceptances([{"KEY": "K1", "column": "age"}]) + + result = mark_reviewed(flags, acceptances, key_name, OUTLIERS) + + assert result.columns == [*flags.columns, REVIEW_STATUS_COL, REVIEW_REASON_COL] + assert result[key_name].to_list() == flags[key_name].to_list() + assert result[REVIEW_STATUS_COL][0] == REVIEWED_BADGE + + @pytest.mark.parametrize("key_name", REVIEW_COLUMNS) + def test_a_key_named_like_a_review_column_is_rejected( + self, outlier_flags, key_name + ): + flags = outlier_flags.rename({"survey_key": key_name}) + + with pytest.raises(ValueError, match="review column"): + mark_reviewed(flags, _acceptances([]), key_name, OUTLIERS) + class TestClearReviewedFlags: def test_reviewed_flags_no_longer_count_as_flags(self, outlier_flags): @@ -338,6 +360,53 @@ def test_reserved_names_are_renamed_on_the_survey_side(self, outlier_flags): assert f"{VIOLATION_TYPE_COL}{SURVEY_COL_SUFFIX * 2}" in table.columns assert f"{VIOLATION_TYPE_COL}{SURVEY_COL_SUFFIX}" in table.columns + def test_survey_review_fields_are_renamed_while_review_columns_are_hidden( + self, outlier_flags + ): + """With "Show reviewed" off, survey text is not read as review state.""" + survey = pl.DataFrame( + { + "survey_key": ["K1", "K2", "K3"], + REVIEW_STATUS_COL: [REVIEWED_BADGE] * 3, + REVIEW_REASON_COL: ["survey text"] * 3, + } + ) + + table = join_survey_columns(survey, outlier_flags, "survey_key", OUTLIERS) + selection = select_flag(table, [0], "survey_key", OUTLIERS) + + assert REVIEW_STATUS_COL not in table.columns + assert REVIEW_REASON_COL not in table.columns + assert table[f"{REVIEW_STATUS_COL}{SURVEY_COL_SUFFIX}"][0] == REVIEWED_BADGE + assert not selection.reviewed + + def test_generated_review_columns_win_over_survey_fields(self, outlier_flags): + """With "Show reviewed" on, badges and reasons come from the log.""" + marked = mark_reviewed( + outlier_flags, + _acceptances([{"KEY": "K1", "column": "age", "reason": "verified"}]), + "survey_key", + OUTLIERS, + ) + survey = pl.DataFrame( + { + "survey_key": ["K1", "K2", "K3"], + REVIEW_STATUS_COL: ["survey status"] * 3, + REVIEW_REASON_COL: ["survey reason"] * 3, + } + ) + + table = join_survey_columns(survey, marked, "survey_key", OUTLIERS) + k1_age = table.filter( + (pl.col("survey_key") == "K1") & (pl.col("column name") == "age") + ) + + assert k1_age[REVIEW_STATUS_COL].to_list() == [REVIEWED_BADGE] + assert k1_age[REVIEW_REASON_COL].to_list() == ["verified"] + assert k1_age[f"{REVIEW_REASON_COL}{SURVEY_COL_SUFFIX}"].to_list() == [ + "survey reason" + ] + def test_row_order_does_not_depend_on_input_order(self, outlier_flags): survey = pl.DataFrame( {"survey_key": ["K1", "K2", "K3"], "enumerator": ["E1", "E2", "E3"]} @@ -437,6 +506,37 @@ def test_no_or_stale_selection_returns_none(self, constraint_table, rows): assert select_flag(constraint_table, rows, "survey_key", CONSTRAINTS) is None +class TestKeyHasConflictingValues: + @pytest.mark.parametrize( + ("ages", "expected"), + [ + ([30, 150, 40], True), + ([150, 150, 40], False), + ([None, 150, 40], True), + ], + ids=["different-values", "same-value", "missing-and-value"], + ) + def test_duplicate_keys(self, ages, expected): + data = pl.DataFrame({"KEY": ["K1", "K1", "K2"], "age": ages}) + + assert key_has_conflicting_values(data, "KEY", "K1", "age") is expected + + def test_unique_key(self): + data = pl.DataFrame({"KEY": ["K1", "K2"], "age": [30, 150]}) + + assert not key_has_conflicting_values(data, "KEY", "K1", "age") + + def test_numeric_key_matches_its_native_value(self): + data = pl.DataFrame({"KEY": [7, 7], "age": [30, 150]}) + + assert key_has_conflicting_values(data, "KEY", 7, "age") + + def test_missing_column_is_not_a_conflict(self): + data = pl.DataFrame({"KEY": ["K1", "K1"], "age": [30, 150]}) + + assert not key_has_conflicting_values(data, "KEY", "K1", "income") + + class TestAllowedActions: def _selection(self, **overrides) -> FlagSelection: values = { diff --git a/tests/processing/test_corrections.py b/tests/processing/test_corrections.py index 1b39bbe2..285b480a 100644 --- a/tests/processing/test_corrections.py +++ b/tests/processing/test_corrections.py @@ -1894,6 +1894,28 @@ def test_severity_is_only_recorded_on_acceptances(self, store, sample_data): assert processor.get_correction_log("survey").is_empty() + @pytest.mark.parametrize( + "overrides", + [{"check_type": "outliers"}, {"severity": "soft"}], + ids=["hard-outlier-acceptance", "unknown-severity"], + ) + def test_invalid_acceptance_severity_is_rejected( + self, store, sample_data, overrides + ): + """Only constraint acceptances can be hard; nothing else is logged.""" + _seed_prep(store, sample_data) + processor = CorrectionProcessor("p1") + + with pytest.raises(ValueError, match="Severity"): + processor.apply_corrections( + alias="survey", + key_col="survey_key", + entries=[self._hard_accept(**overrides)], + source="outliers", + ) + + assert processor.get_correction_log("survey").is_empty() + def test_legacy_logs_load_with_null_severity(self, store, sample_corrections_log): store[("p1", "logs", "corr_log_survey")] = sample_corrections_log @@ -1987,6 +2009,31 @@ def test_excludes_acceptances_and_removed_rows(self, store, sample_data): assert processor.get_active_corrections("survey", "survey_key").is_empty() + def test_a_correction_that_failed_to_reapply_is_inactive(self, store, sample_data): + """Prep data that holds the new value doesn't make a failed correction + look applied. + """ + _seed_prep(store, sample_data) + processor = CorrectionProcessor("p1") + self._correct(processor, self._modify("key1", "age", 26, 25)) + # Prep now supplies 26 itself, so replay finds 26 where the + # correction expects 25 and rejects it. + _seed_prep( + store, + sample_data.with_columns( + pl.when(pl.col("survey_key") == "key1") + .then(26) + .otherwise(pl.col("age")) + .alias("age") + ), + ) + + failures = processor.refresh_existing_corrected_data("survey") + + assert len(failures) == 1 + assert processor.get_corrected_data("survey")["age"][0] == 26 + assert processor.get_active_corrections("survey", "survey_key").is_empty() + def test_empty_log_returns_no_corrections(self, store, sample_data): _seed_prep(store, sample_data)