From 308591888af6ff34cb25be2fbafd508786a7045f Mon Sep 17 00:00:00 2001 From: ppcvote Date: Thu, 13 Aug 2026 16:17:26 +0800 Subject: [PATCH] fix: make ascii_tree's optional df argument usable `ascii_tree(dt, df)` raised ValueError for every non-None df, so the second parameter in its own signature could never be used. helpers.py:380 tested the sentinel with `==`. On a DataFrame that is elementwise, so it returns a same-shaped boolean frame and the enclosing `if` calls DataFrame.__bool__, which pandas raises from by design. The default path survived only because `None == None` is a plain scalar True, and every caller in the repository and both README examples omit df, so nothing exercised it. base.py:706 re-exports the function, so both public routes failed identically. Dropping the "row" column is no longer done in place. That in-place drop was harmless only while df could not be supplied, because the frame always belonged to this function. Making the argument work makes the mutation reachable, and a caller who passes a frame carrying a "row" column would silently lose the column from their own object. Verified before the change: caller columns went from ['row', ...] to [...] after the call. Adds src/test/decision_tables/test_helpers.py; ascii_tree had no test coverage. Five tests, four of which fail on unmodified main. The fifth covers the df-omitted path and passes either way, so the suite is not vacuous. resolves #1224 --- src/ssvc/decision_tables/helpers.py | 6 +- src/test/decision_tables/test_helpers.py | 74 ++++++++++++++++++++++++ 2 files changed, 78 insertions(+), 2 deletions(-) create mode 100644 src/test/decision_tables/test_helpers.py diff --git a/src/ssvc/decision_tables/helpers.py b/src/ssvc/decision_tables/helpers.py index 4326236e9..1700418cc 100644 --- a/src/ssvc/decision_tables/helpers.py +++ b/src/ssvc/decision_tables/helpers.py @@ -377,11 +377,13 @@ def ascii_tree(dt: DecisionTable, df: pd.DataFrame | None = None) -> str: Reads a Pandas data frame, builds a decision tree, and returns its ASCII representation. """ # Check for the optional 'row' column and drop it if it exists. - if df == None: + if df is None: df = decision_table_to_longform_df(dt) if "row" in df.columns: - df.drop(columns="row", inplace=True) + # Not in place: df may belong to the caller, and dropping a column from + # under them is not something a read-only rendering helper should do. + df = df.drop(columns="row") # Separate feature columns from the outcome column. feature_cols = list(df.columns[:-1]) diff --git a/src/test/decision_tables/test_helpers.py b/src/test/decision_tables/test_helpers.py new file mode 100644 index 000000000..94c7ea86b --- /dev/null +++ b/src/test/decision_tables/test_helpers.py @@ -0,0 +1,74 @@ +# Copyright (c) 2026 Carnegie Mellon University. +# NO WARRANTY. THIS CARNEGIE MELLON UNIVERSITY AND SOFTWARE +# ENGINEERING INSTITUTE MATERIAL IS FURNISHED ON AN "AS-IS" BASIS. +# CARNEGIE MELLON UNIVERSITY MAKES NO WARRANTIES OF ANY KIND, +# EITHER EXPRESSED OR IMPLIED, AS TO ANY MATTER INCLUDING, BUT +# NOT LIMITED TO, WARRANTY OF FITNESS FOR PURPOSE OR +# MERCHANTABILITY, EXCLUSIVITY, OR RESULTS OBTAINED FROM USE +# OF THE MATERIAL. CARNEGIE MELLON UNIVERSITY DOES NOT MAKE +# ANY WARRANTY OF ANY KIND WITH RESPECT TO FREEDOM FROM +# PATENT, TRADEMARK, OR COPYRIGHT INFRINGEMENT. +# Licensed under a MIT (SEI)-style license, please see LICENSE or contact +# permission@sei.cmu.edu for full terms. +# [DISTRIBUTION STATEMENT A] This material has been approved for +# public release and unlimited distribution. Please see Copyright notice +# for non-US Government use and distribution. +# This Software includes and/or makes use of Third-Party Software each +# subject to its own license. +# DM24-0278 +import unittest + +from ssvc.decision_tables.base import ascii_tree as base_ascii_tree +from ssvc.decision_tables.example.to_play import LATEST as EXAMPLE_DT +from ssvc.decision_tables.helpers import ( + ascii_tree, + decision_table_to_longform_df, +) + + +class TestAsciiTree(unittest.TestCase): + def setUp(self) -> None: + self.dt = EXAMPLE_DT + + def test_df_omitted(self) -> None: + # The path every caller in the repository takes. + self.assertGreater(len(ascii_tree(self.dt).splitlines()), 0) + + def test_df_supplied(self) -> None: + # Passing the frame is what the signature invites, and `df == None` + # made it raise ValueError from DataFrame.__bool__ for every frame. + df = decision_table_to_longform_df(self.dt) + self.assertEqual(ascii_tree(self.dt, df), ascii_tree(self.dt)) + + def test_df_supplied_via_base(self) -> None: + # base re-exports the helper, so the same call has two public routes. + df = decision_table_to_longform_df(self.dt) + self.assertEqual( + base_ascii_tree(self.dt, df), base_ascii_tree(self.dt) + ) + + def test_caller_frame_is_not_modified(self) -> None: + # The "row" column was dropped in place. That only ever touched the + # frame this function built itself while `df` could not be supplied; + # once it can, an in-place drop takes a column off the caller's object. + df = decision_table_to_longform_df(self.dt) + df.insert(0, "row", range(len(df))) + columns_before = list(df.columns) + + ascii_tree(self.dt, df) + + self.assertEqual(list(df.columns), columns_before) + + def test_row_column_is_excluded_from_the_tree(self) -> None: + # Dropping "row" must still happen, just not on the caller's frame. + df = decision_table_to_longform_df(self.dt) + without_row = ascii_tree(self.dt, df) + + df_with_row = decision_table_to_longform_df(self.dt) + df_with_row.insert(0, "row", range(len(df_with_row))) + + self.assertEqual(ascii_tree(self.dt, df_with_row), without_row) + + +if __name__ == "__main__": + unittest.main()