From 38398528d5307d03eb225f760fa6cea4aab64e06 Mon Sep 17 00:00:00 2001 From: harryswift01 Date: Fri, 27 Jun 2025 10:51:21 +0100 Subject: [PATCH 1/3] resolving issues mentioned in issue #114 with `water_entropy` argument beign ignored: - within the `arg_config_manager.py` a new function `str2bool` ensures all types of boolean are accpeted - `setup_argsparse` has been updated to ensure if `property` is a type `bool` it is handled correctly - additional test cases to ensure that the modified `setup_argsparse` and the `str2bool` function works as intended --- CodeEntropy/config/arg_config_manager.py | 43 ++++++++++- config.yaml | 2 +- .../test_arg_config_manager.py | 71 +++++++++++++++++++ 3 files changed, 113 insertions(+), 3 deletions(-) diff --git a/CodeEntropy/config/arg_config_manager.py b/CodeEntropy/config/arg_config_manager.py index 1bd9fbf..6c7ebc4 100644 --- a/CodeEntropy/config/arg_config_manager.py +++ b/CodeEntropy/config/arg_config_manager.py @@ -85,6 +85,34 @@ def load_config(self, file_path): return config + def str2bool(self, value): + """ + Convert a string or boolean input into a boolean value. + + Accepts common string representations of boolean values such as: + - True values: "true", "t", "yes", "1" + - False values: "false", "f", "no", "0" + + If the input is already a boolean, it is returned as-is. + Raises: + argparse.ArgumentTypeError: If the input cannot be interpreted as a boolean. + + Args: + value (str or bool): The input value to convert. + + Returns: + bool: The corresponding boolean value. + """ + if isinstance(value, bool): + return value + value = value.lower() + if value in {"true", "t", "yes", "1"}: + return True + elif value in {"false", "f", "no", "0"}: + return False + else: + raise argparse.ArgumentTypeError("Boolean value expected (True/False).") + def setup_argparse(self): """Setup argument parsing dynamically based on arg_map.""" parser = argparse.ArgumentParser( @@ -92,8 +120,19 @@ def setup_argparse(self): ) for arg, properties in self.arg_map.items(): - kwargs = {key: properties[key] for key in properties if key != "help"} - parser.add_argument(f"--{arg}", **kwargs, help=properties.get("help")) + help_text = properties.get("help", "") + default = properties.get("default", None) + + if properties.get("type") == bool: + parser.add_argument( + f"--{arg}", + type=self.str2bool, + default=default, + help=f"{help_text} (default: {default})", + ) + else: + kwargs = {k: v for k, v in properties.items() if k != "help"} + parser.add_argument(f"--{arg}", **kwargs, help=help_text) return parser diff --git a/config.yaml b/config.yaml index 2a7ba86..f76edf8 100644 --- a/config.yaml +++ b/config.yaml @@ -12,4 +12,4 @@ run1: thread: output_file: force_partitioning: - disable_water_entropy: + water_entropy: \ No newline at end of file diff --git a/tests/test_CodeEntropy/test_arg_config_manager.py b/tests/test_CodeEntropy/test_arg_config_manager.py index 427275e..1c2acdc 100644 --- a/tests/test_CodeEntropy/test_arg_config_manager.py +++ b/tests/test_CodeEntropy/test_arg_config_manager.py @@ -137,6 +137,77 @@ def test_setup_argparse(self, mock_args): self.assertEqual(args.top_traj_file, ["/path/to/tpr", "/path/to/trr"]) self.assertEqual(args.selection_string, "all") + @patch( + "argparse.ArgumentParser.parse_args", + return_value=MagicMock( + top_traj_file=["/path/to/tpr", "/path/to/trr"], + start=10, + water_entropy=False, + ), + ) + def test_setup_argparse_false_boolean(self, mock_args): + """ + Test that non-boolean arguments are parsed correctly. + """ + arg_config = ConfigManager() + parser = arg_config.setup_argparse() + args = parser.parse_args() + + self.assertEqual(args.top_traj_file, ["/path/to/tpr", "/path/to/trr"]) + self.assertEqual(args.start, 10) + self.assertFalse(args.water_entropy) + + def test_str2bool_true_variants(self): + """Test that various string representations of True are correctly parsed.""" + arg_config = ConfigManager() + + self.assertTrue(arg_config.str2bool("true")) + self.assertTrue(arg_config.str2bool("True")) + self.assertTrue(arg_config.str2bool("t")) + self.assertTrue(arg_config.str2bool("yes")) + self.assertTrue(arg_config.str2bool("1")) + + def test_str2bool_false_variants(self): + """Test that various string representations of False are correctly parsed.""" + arg_config = ConfigManager() + + self.assertFalse(arg_config.str2bool("false")) + self.assertFalse(arg_config.str2bool("False")) + self.assertFalse(arg_config.str2bool("f")) + self.assertFalse(arg_config.str2bool("no")) + self.assertFalse(arg_config.str2bool("0")) + + def test_str2bool_boolean_passthrough(self): + """Test that boolean values passed directly are returned unchanged.""" + arg_config = ConfigManager() + + self.assertTrue(arg_config.str2bool(True)) + self.assertFalse(arg_config.str2bool(False)) + + def test_str2bool_invalid_input(self): + """Test that invalid string inputs raise an ArgumentTypeError.""" + arg_config = ConfigManager() + + with self.assertRaises(Exception) as context: + arg_config.str2bool("maybe") + self.assertIn("Boolean value expected", str(context.exception)) + + def test_str2bool_empty_string(self): + """Test that an empty string raises an ArgumentTypeError.""" + arg_config = ConfigManager() + + with self.assertRaises(Exception) as context: + arg_config.str2bool("") + self.assertIn("Boolean value expected", str(context.exception)) + + def test_str2bool_unexpected_number(self): + """Test that unexpected numeric strings raise an ArgumentTypeError.""" + arg_config = ConfigManager() + + with self.assertRaises(Exception) as context: + arg_config.str2bool("2") + self.assertIn("Boolean value expected", str(context.exception)) + def test_cli_overrides_defaults(self): """ Test if CLI parameters override default values. From 1794f68c316106db2c1c4be245d1e90af9160b50 Mon Sep 17 00:00:00 2001 From: harryswift01 Date: Fri, 27 Jun 2025 11:07:21 +0100 Subject: [PATCH 2/3] resolving one of the comments within the issue #114, all input options are now visible on the output --- CodeEntropy/run.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CodeEntropy/run.py b/CodeEntropy/run.py index 5aa76ee..0febe21 100644 --- a/CodeEntropy/run.py +++ b/CodeEntropy/run.py @@ -127,7 +127,7 @@ def run_entropy_workflow(self): # Log all inputs for the current run logger.info(f"All input for {run_name}") for arg in vars(args): - logger.info(f" {arg}: {getattr(args, arg) or ''}") + logger.info(f" {arg}: {getattr(args, arg)}") # Load MDAnalysis Universe tprfile = args.top_traj_file[0] From a358aca87c9f835ea8fa4e8e7b346fa0c9c0a965 Mon Sep 17 00:00:00 2001 From: harryswift01 Date: Fri, 27 Jun 2025 11:59:36 +0100 Subject: [PATCH 3/3] addressing issue #83, ensuring there are sensible checks in place for the input parameters --- CodeEntropy/config/arg_config_manager.py | 59 +++++++++++ CodeEntropy/run.py | 2 + .../test_arg_config_manager.py | 97 +++++++++++++++++++ 3 files changed, 158 insertions(+) diff --git a/CodeEntropy/config/arg_config_manager.py b/CodeEntropy/config/arg_config_manager.py index 6c7ebc4..373dd9d 100644 --- a/CodeEntropy/config/arg_config_manager.py +++ b/CodeEntropy/config/arg_config_manager.py @@ -185,3 +185,62 @@ def merge_configs(self, args, run_config): handler.setLevel(logging.INFO) return args + + def input_parameters_validation(self, u, args): + """Check the validity of the user inputs against sensible values""" + + self._check_input_start(u, args) + self._check_input_end(u, args) + self._check_input_step(args) + self._check_input_bin_width(args) + self._check_input_temperature(args) + self._check_input_force_partitioning(args) + + def _check_input_start(self, u, args): + """Check that the input does not exceed the length of the trajectory.""" + if args.start > len(u.trajectory): + raise ValueError( + f"Invalid 'start' value: {args.start}. It exceeds the trajectory length" + " of {len(u.trajectory)}." + ) + + def _check_input_end(self, u, args): + """Check that the end index does not exceed the trajectory length.""" + if args.end > len(u.trajectory): + raise ValueError( + f"Invalid 'end' value: {args.end}. It exceeds the trajectory length of" + " {len(u.trajectory)}." + ) + + def _check_input_step(self, args): + """Check that the step value is non-negative.""" + if args.step < 0: + logger.warning( + f"Negative 'step' value provided: {args.step}. This may lead to" + " unexpected behavior." + ) + + def _check_input_bin_width(self, args): + """Check that the bin width is within the valid range [0, 360].""" + if args.bin_width < 0 or args.bin_width > 360: + raise ValueError( + f"Invalid 'bin_width': {args.bin_width}. It must be between 0 and 360" + " degrees." + ) + + def _check_input_temperature(self, args): + """Check that the temperature is non-negative.""" + if args.temperature < 0: + raise ValueError( + f"Invalid 'temperature': {args.temperature}. Temperature cannot be" + " below 0." + ) + + def _check_input_force_partitioning(self, args): + """Warn if force partitioning is not set to the default value.""" + default_value = arg_map["force_partitioning"]["default"] + if args.force_partitioning != default_value: + logger.warning( + f"'force_partitioning' is set to {args.force_partitioning}," + " which differs from the default ({default_value})." + ) diff --git a/CodeEntropy/run.py b/CodeEntropy/run.py index 0febe21..e89d259 100644 --- a/CodeEntropy/run.py +++ b/CodeEntropy/run.py @@ -135,6 +135,8 @@ def run_entropy_workflow(self): logger.debug(f"Loading Universe with {tprfile} and {trrfile}") u = mda.Universe(tprfile, trrfile) + self._config_manager.input_parameters_validation(u, args) + # Create LevelManager instance level_manager = LevelManager() diff --git a/tests/test_CodeEntropy/test_arg_config_manager.py b/tests/test_CodeEntropy/test_arg_config_manager.py index 1c2acdc..664116f 100644 --- a/tests/test_CodeEntropy/test_arg_config_manager.py +++ b/tests/test_CodeEntropy/test_arg_config_manager.py @@ -494,6 +494,103 @@ def test_empty_yaml_config(self, mock_exists, mock_file): self.assertIsInstance(config, dict) self.assertEqual(config, {}) + def test_input_parameters_validation_all_valid(self): + """Test that input_parameters_validation passes with all valid inputs.""" + manager = ConfigManager() + u = MagicMock() + u.trajectory = [0] * 100 + + args = MagicMock( + start=10, + end=90, + step=1, + bin_width=30, + temperature=298.0, + force_partitioning=0.5, + ) + + with patch.dict( + "CodeEntropy.config.arg_config_manager.arg_map", + {"force_partitioning": {"default": 0.5}}, + ): + manager.input_parameters_validation(u, args) + + def test_check_input_start_valid(self): + """Test that a valid 'start' value does not raise an error.""" + args = MagicMock(start=50) + u = MagicMock() + u.trajectory = [0] * 100 + ConfigManager()._check_input_start(u, args) + + def test_check_input_start_invalid(self): + """Test that an invalid 'start' value raises a ValueError.""" + args = MagicMock(start=150) + u = MagicMock() + u.trajectory = [0] * 100 + with self.assertRaises(ValueError): + ConfigManager()._check_input_start(u, args) + + def test_check_input_end_valid(self): + """Test that a valid 'end' value does not raise an error.""" + args = MagicMock(end=100) + u = MagicMock() + u.trajectory = [0] * 100 + ConfigManager()._check_input_end(u, args) + + def test_check_input_end_invalid(self): + """Test that an 'end' value exceeding trajectory length raises a ValueError.""" + args = MagicMock(end=101) + u = MagicMock() + u.trajectory = [0] * 100 + with self.assertRaises(ValueError): + ConfigManager()._check_input_end(u, args) + + @patch("CodeEntropy.config.arg_config_manager.logger") + def test_check_input_step_negative(self, mock_logger): + """Test that a negative 'step' value triggers a warning.""" + args = MagicMock(step=-1) + ConfigManager()._check_input_step(args) + mock_logger.warning.assert_called_once() + + def test_check_input_bin_width_valid(self): + """Test that a valid 'bin_width' value does not raise an error.""" + args = MagicMock(bin_width=180) + ConfigManager()._check_input_bin_width(args) + + def test_check_input_bin_width_invalid_low(self): + """Test that a negative 'bin_width' value raises a ValueError.""" + args = MagicMock(bin_width=-10) + with self.assertRaises(ValueError): + ConfigManager()._check_input_bin_width(args) + + def test_check_input_bin_width_invalid_high(self): + """Test that a 'bin_width' value above 360 raises a ValueError.""" + args = MagicMock(bin_width=400) + with self.assertRaises(ValueError): + ConfigManager()._check_input_bin_width(args) + + def test_check_input_temperature_valid(self): + """Test that a valid 'temperature' value does not raise an error.""" + args = MagicMock(temperature=298.0) + ConfigManager()._check_input_temperature(args) + + def test_check_input_temperature_invalid(self): + """Test that a negative 'temperature' value raises a ValueError.""" + args = MagicMock(temperature=-5) + with self.assertRaises(ValueError): + ConfigManager()._check_input_temperature(args) + + @patch("CodeEntropy.config.arg_config_manager.logger") + def test_check_input_force_partitioning_warning(self, mock_logger): + """Test that a non-default 'force_partitioning' value triggers a warning.""" + args = MagicMock(force_partitioning=0.7) + with patch.dict( + "CodeEntropy.config.arg_config_manager.arg_map", + {"force_partitioning": {"default": 0.5}}, + ): + ConfigManager()._check_input_force_partitioning(args) + mock_logger.warning.assert_called_once() + if __name__ == "__main__": unittest.main()