From 8db38dc8318c98584ad7eaa151b167d19f61ab23 Mon Sep 17 00:00:00 2001 From: jinukuntlaakhilakumargoud-web Date: Mon, 2 Mar 2026 17:56:18 +0530 Subject: [PATCH 1/2] Refactor plot saving into a separate function and prevent saving blank images when suppressed (closes #357) --- acro/acro_tables.py | 107 +++++++++++++++++++++++-------------------- test/test_initial.py | 4 +- 2 files changed, 60 insertions(+), 51 deletions(-) diff --git a/acro/acro_tables.py b/acro/acro_tables.py index ce827c79..05a9974b 100644 --- a/acro/acro_tables.py +++ b/acro/acro_tables.py @@ -59,6 +59,43 @@ def mode_aggfunc(values) -> Series: SURVIVAL_THRESHOLD: int = 10 +import os +import matplotlib.pyplot as plt + +def _save_plot(filename: str) -> str | None: + """Save the current plot to the acro_artifacts directory with a unique name. + + Parameters + ---------- + filename : str + The base name of the file where the plot will be saved. + + Returns + ------- + str | None + The unique filename where the plot was saved, or None if invalid extension. + """ + try: + os.makedirs("acro_artifacts") + logger.debug("Directory acro_artifacts created successfully") + except FileExistsError: # pragma: no cover + logger.debug("Directory acro_artifacts already exists") + + filename, extension = os.path.splitext(filename) + if not extension: # pragma: no cover + logger.info("Please provide a valid file extension") + return None + + increment_number = 0 + while os.path.exists( + f"acro_artifacts/{filename}_{increment_number}{extension}" + ): # pragma: no cover + increment_number += 1 + unique_filename = f"acro_artifacts/{filename}_{increment_number}{extension}" + + plt.savefig(unique_filename) + return unique_filename + class Tables: """Creates tabular data. @@ -545,32 +582,20 @@ def survival_plot( # pylint: disable=too-many-arguments self, survival_table, survival_func, filename, status, sdc, command, summary ): """Create the survival plot according to the status of suppressing.""" - if self.suppress: - survival_table = _rounded_survival_table(survival_table) - plot = survival_table.plot(y="rounded_survival_fun", xlim=0, ylim=0) - else: # pragma: no cover - plot = survival_func.plot() - - try: - os.makedirs("acro_artifacts") - logger.debug("Directory acro_artifacts created successfully") - except FileExistsError: # pragma: no cover - logger.debug("Directory acro_artifacts already exists") - - # create a unique filename with number to avoid overwrite - filename, extension = os.path.splitext(filename) - if not extension: # pragma: no cover - logger.info("Please provide a valid file extension") - return None # pragma: no cover - increment_number = 0 - while os.path.exists( - f"acro_artifacts/{filename}_{increment_number}{extension}" - ): # pragma: no cover - increment_number += 1 - unique_filename = f"acro_artifacts/{filename}_{increment_number}{extension}" - - # save the plot to the acro artifacts directory - plt.savefig(unique_filename) + if self.suppress and status == "fail": + logger.warning("Survival plot will not be shown as it is disclosive.") + unique_filename = None + plot = None + else: + if self.suppress: + survival_table = _rounded_survival_table(survival_table) + plot = survival_table.plot(y="rounded_survival_fun", xlim=0, ylim=0) + else: # pragma: no cover + plot = survival_func.plot() + + unique_filename = _save_plot(filename) + + output_list = [os.path.normpath(unique_filename)] if unique_filename else [] # record output self.results.add( @@ -581,7 +606,7 @@ def survival_plot( # pylint: disable=too-many-arguments command=command, summary=summary, outcome=pd.DataFrame(), - output=[os.path.normpath(unique_filename)], + output=output_list, ) return (plot, unique_filename) @@ -694,6 +719,7 @@ def hist( # pylint: disable=too-many-arguments,too-many-locals "Histogram will not be shown as the %s column is disclosive.", column, ) + unique_filename = None else: # pragma: no cover data.hist( column=column, @@ -713,6 +739,7 @@ def hist( # pylint: disable=too-many-arguments,too-many-locals legend=legend, **kwargs, ) + unique_filename = _save_plot(filename) else: status = "review" data.hist( @@ -733,6 +760,8 @@ def hist( # pylint: disable=too-many-arguments,too-many-locals legend=legend, **kwargs, ) + unique_filename = _save_plot(filename) + logger.info("status: %s", status) # create the summary @@ -744,27 +773,7 @@ def hist( # pylint: disable=too-many-arguments,too-many-locals f"The maximum value of the {column} column is: {max_value}" ) - # create the acro_artifacts directory to save the plot in it - try: - os.makedirs("acro_artifacts") - logger.debug("Directory acro_artifacts created successfully") - except FileExistsError: # pragma: no cover - logger.debug("Directory acro_artifacts already exists") - - # create a unique filename with number to avoid overwrite - filename, extension = os.path.splitext(filename) - if not extension: # pragma: no cover - logger.info("Please provide a valid file extension") - return None - increment_number = 0 - while os.path.exists( - f"acro_artifacts/{filename}_{increment_number}{extension}" - ): # pragma: no cover - increment_number += 1 - unique_filename = f"acro_artifacts/{filename}_{increment_number}{extension}" - - # save the plot to the acro artifacts directory - plt.savefig(unique_filename) + output_list = [os.path.normpath(unique_filename)] if unique_filename else [] # record output self.results.add( @@ -775,7 +784,7 @@ def hist( # pylint: disable=too-many-arguments,too-many-locals command=command, summary=summary, outcome=pd.DataFrame(), - output=[os.path.normpath(unique_filename)], + output=output_list, ) return unique_filename diff --git a/test/test_initial.py b/test/test_initial.py index 7c796188..7b8c5d4f 100644 --- a/test/test_initial.py +++ b/test/test_initial.py @@ -1079,11 +1079,11 @@ def test_histogram_disclosive(data, acro, caplog): """Test a discolsive histogram.""" filename = os.path.normpath("acro_artifacts/histogram_0.png") _ = acro.hist(data, "inc_grants") - assert os.path.exists(filename) + assert not os.path.exists(filename) acro.add_exception("output_0", "Let me have it") results: Records = acro.finalise(path=PATH) output_0 = results.get_index(0) - assert output_0.output == [filename] + assert output_0.output == [] assert ( "Histogram will not be shown as the inc_grants column is disclosive." in caplog.text From 281739c9b5362a10d6c1c00da2c871bd9d04ef79 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Mon, 2 Mar 2026 12:28:56 +0000 Subject: [PATCH 2/2] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- acro/acro_tables.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/acro/acro_tables.py b/acro/acro_tables.py index 05a9974b..b883d65f 100644 --- a/acro/acro_tables.py +++ b/acro/acro_tables.py @@ -59,9 +59,6 @@ def mode_aggfunc(values) -> Series: SURVIVAL_THRESHOLD: int = 10 -import os -import matplotlib.pyplot as plt - def _save_plot(filename: str) -> str | None: """Save the current plot to the acro_artifacts directory with a unique name. @@ -96,6 +93,7 @@ def _save_plot(filename: str) -> str | None: plt.savefig(unique_filename) return unique_filename + class Tables: """Creates tabular data.