From 8e5baa38da87baeda38aec13a9e50068c8ef773b Mon Sep 17 00:00:00 2001 From: Timothy Willard <9395586+TimothyWillard@users.noreply.github.com> Date: Mon, 9 Dec 2024 15:19:04 -0500 Subject: [PATCH 01/18] Correct formula for poisson log-likelihood Per this comment https://github.com/HopkinsIDD/flepiMoP/pull/375#discussion_r1874023191 corrected the formula for the poisson log-likelihood, including swapping the `ymodel` and `ydata` so it matches prior poisson likelihood. Added a unit test case to demonstrate this change is to restore current behavior. --- .../gempyor_pkg/src/gempyor/statistics.py | 4 +- .../tests/statistics/test_statistic_class.py | 48 +++++++++++++++++++ 2 files changed, 50 insertions(+), 2 deletions(-) diff --git a/flepimop/gempyor_pkg/src/gempyor/statistics.py b/flepimop/gempyor_pkg/src/gempyor/statistics.py index 7b62c608e..3d85c2693 100644 --- a/flepimop/gempyor_pkg/src/gempyor/statistics.py +++ b/flepimop/gempyor_pkg/src/gempyor/statistics.py @@ -244,8 +244,8 @@ def llik(self, model_data: xr.DataArray, gt_data: xr.DataArray) -> xr.DataArray: """ dist_map = { - "pois": lambda ymodel, ydata: -(ymodel + 1) - + ydata * np.log(ymodel + 1) + "pois": lambda ydata, ymodel: -ymodel + + (ydata * np.log(ymodel)) - gammaln(ydata + 1), # >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> # OLD: # TODO: Swap out in favor of NEW diff --git a/flepimop/gempyor_pkg/tests/statistics/test_statistic_class.py b/flepimop/gempyor_pkg/tests/statistics/test_statistic_class.py index 781820165..cc4b79f8c 100644 --- a/flepimop/gempyor_pkg/tests/statistics/test_statistic_class.py +++ b/flepimop/gempyor_pkg/tests/statistics/test_statistic_class.py @@ -228,11 +228,59 @@ def simple_valid_resample_and_scale_factory() -> MockStatisticInput: ) +def simple_valid_factory_with_pois() -> MockStatisticInput: + data_coords = { + "date": pd.date_range(date(2024, 1, 1), date(2024, 1, 10)), + "subpop": ["01", "02", "03"], + } + data_dim = [len(v) for v in data_coords.values()] + model_data = xr.Dataset( + data_vars={ + "incidH": ( + list(data_coords.keys()), + np.random.poisson(lam=20.0, size=data_dim), + ), + "incidD": ( + list(data_coords.keys()), + np.random.poisson(lam=20.0, size=data_dim), + ), + }, + coords=data_coords, + ) + gt_data = xr.Dataset( + data_vars={ + "incidH": ( + list(data_coords.keys()), + np.random.poisson(lam=20.0, size=data_dim), + ), + "incidD": ( + list(data_coords.keys()), + np.random.poisson(lam=20.0, size=data_dim), + ), + }, + coords=data_coords, + ) + return MockStatisticInput( + "total_hospitalizations", + { + "name": "sum_hospitalizations", + "sim_var": "incidH", + "data_var": "incidH", + "remove_na": True, + "add_one": True, + "likelihood": {"dist": "pois"}, + }, + model_data=model_data, + gt_data=gt_data, + ) + + all_valid_factories = [ (simple_valid_factory), (simple_valid_resample_factory), (simple_valid_resample_factory), (simple_valid_resample_and_scale_factory), + (simple_valid_factory_with_pois), ] From 10e5d9893e340c90bb2dca52d060a7176a685c60 Mon Sep 17 00:00:00 2001 From: Timothy Willard <9395586+TimothyWillard@users.noreply.github.com> Date: Mon, 9 Dec 2024 16:29:57 -0500 Subject: [PATCH 02/18] Add test case with poisson and zeros Added a test case with poisson log-likelihood and zero valued data with the `zero_to_one` flag set to `True`. --- .../tests/statistics/test_statistic_class.py | 53 ++++++++++++++++++- 1 file changed, 51 insertions(+), 2 deletions(-) diff --git a/flepimop/gempyor_pkg/tests/statistics/test_statistic_class.py b/flepimop/gempyor_pkg/tests/statistics/test_statistic_class.py index cc4b79f8c..c0397ecb2 100644 --- a/flepimop/gempyor_pkg/tests/statistics/test_statistic_class.py +++ b/flepimop/gempyor_pkg/tests/statistics/test_statistic_class.py @@ -275,12 +275,48 @@ def simple_valid_factory_with_pois() -> MockStatisticInput: ) +def simple_valid_factory_with_pois_with_some_zeros() -> MockStatisticInput: + mock_input = simple_valid_factory_with_pois() + + mock_input.config["zero_to_one"] = True + + mock_input.model_data["incidH"].loc[ + { + "date": mock_input.model_data.coords["date"][0], + "subpop": mock_input.model_data.coords["subpop"][0], + } + ] = 0 + + mock_input.gt_data["incidD"].loc[ + { + "date": mock_input.gt_data.coords["date"][0], + "subpop": mock_input.gt_data.coords["subpop"][0], + } + ] = 0 + + mock_input.model_data["incidH"].loc[ + { + "date": mock_input.model_data.coords["date"][1], + "subpop": mock_input.model_data.coords["subpop"][1], + } + ] = 0 + mock_input.gt_data["incidH"].loc[ + { + "date": mock_input.gt_data.coords["date"][1], + "subpop": mock_input.gt_data.coords["subpop"][1], + } + ] = 0 + + return mock_input + + all_valid_factories = [ (simple_valid_factory), (simple_valid_resample_factory), (simple_valid_resample_factory), (simple_valid_resample_and_scale_factory), (simple_valid_factory_with_pois), + (simple_valid_factory_with_pois_with_some_zeros), ] @@ -549,8 +585,21 @@ def test_llik(self, factory: Callable[[], MockStatisticInput]) -> None: assert np.allclose( log_likelihood.values, scipy.stats.poisson.logpmf( - mock_inputs.gt_data[mock_inputs.config["data_var"]].values, - mock_inputs.model_data[mock_inputs.config["data_var"]].values, + np.where( + mock_inputs.config.get("zero_to_one", False) + & (mock_inputs.gt_data[mock_inputs.config["data_var"]].values == 0), + 1, + mock_inputs.gt_data[mock_inputs.config["data_var"]].values, + ), + np.where( + mock_inputs.config.get("zero_to_one", False) + & ( + mock_inputs.model_data[mock_inputs.config["data_var"]].values + == 0 + ), + 1, + mock_inputs.model_data[mock_inputs.config["data_var"]].values, + ), ), ) elif dist_name in {"norm", "norm_cov"}: From 14d9bf3a4d343b7c9fb1f80b51b9a0b6ec78c433 Mon Sep 17 00:00:00 2001 From: Timothy Willard <9395586+TimothyWillard@users.noreply.github.com> Date: Tue, 10 Dec 2024 11:49:50 -0500 Subject: [PATCH 03/18] Simplify poisson LL tests with only one variable --- .../tests/statistics/test_statistic_class.py | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/flepimop/gempyor_pkg/tests/statistics/test_statistic_class.py b/flepimop/gempyor_pkg/tests/statistics/test_statistic_class.py index c0397ecb2..008b5073d 100644 --- a/flepimop/gempyor_pkg/tests/statistics/test_statistic_class.py +++ b/flepimop/gempyor_pkg/tests/statistics/test_statistic_class.py @@ -240,10 +240,6 @@ def simple_valid_factory_with_pois() -> MockStatisticInput: list(data_coords.keys()), np.random.poisson(lam=20.0, size=data_dim), ), - "incidD": ( - list(data_coords.keys()), - np.random.poisson(lam=20.0, size=data_dim), - ), }, coords=data_coords, ) @@ -253,10 +249,6 @@ def simple_valid_factory_with_pois() -> MockStatisticInput: list(data_coords.keys()), np.random.poisson(lam=20.0, size=data_dim), ), - "incidD": ( - list(data_coords.keys()), - np.random.poisson(lam=20.0, size=data_dim), - ), }, coords=data_coords, ) @@ -287,10 +279,10 @@ def simple_valid_factory_with_pois_with_some_zeros() -> MockStatisticInput: } ] = 0 - mock_input.gt_data["incidD"].loc[ + mock_input.gt_data["incidH"].loc[ { - "date": mock_input.gt_data.coords["date"][0], - "subpop": mock_input.gt_data.coords["subpop"][0], + "date": mock_input.gt_data.coords["date"][2], + "subpop": mock_input.gt_data.coords["subpop"][2], } ] = 0 From b21cfccca7a3a0c7bae7b7df4ac08a209834a4a5 Mon Sep 17 00:00:00 2001 From: Timothy Willard <9395586+TimothyWillard@users.noreply.github.com> Date: Tue, 10 Dec 2024 15:22:33 -0500 Subject: [PATCH 04/18] `flepimop patch` unit test for `seir::parameters` Added a unit test to demonstrate the issue described in https://github.com/HopkinsIDD/flepiMoP/pull/375#issuecomment-2532668816 with respect to the `seir::parameters` section. Appears to be related to if the parameters are a list or a dict. --- .../tests/cli/test_flepimop_patch_cli.py | 76 +++++++++++++++++++ 1 file changed, 76 insertions(+) create mode 100644 flepimop/gempyor_pkg/tests/cli/test_flepimop_patch_cli.py diff --git a/flepimop/gempyor_pkg/tests/cli/test_flepimop_patch_cli.py b/flepimop/gempyor_pkg/tests/cli/test_flepimop_patch_cli.py new file mode 100644 index 000000000..72d0134b9 --- /dev/null +++ b/flepimop/gempyor_pkg/tests/cli/test_flepimop_patch_cli.py @@ -0,0 +1,76 @@ +from pathlib import Path +from typing import Any + +from click.testing import CliRunner +import pytest +import yaml + +from gempyor.cli import patch + + +@pytest.mark.parametrize( + ("data_one", "data_two", "expected_parameters"), + ( + ( + { + "seir": { + "parameters": { + "beta": {"value": 1.2}, + } + } + }, + { + "seir": { + "parameters": { + "gamma": {"value": 3.4}, + } + } + }, + {"gamma": {"value": 3.4}}, + ), + ( + { + "seir": { + "parameters": { + "sigma": {"value": 5.6}, + "gamma": {"value": 7.8}, + } + } + }, + { + "seir": { + "parameters": { + "gamma": {"value": 3.4}, + } + } + }, + {"gamma": {"value": 3.4}}, + ), + ), +) +def test_patch_seir_parameters_behavior( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + data_one: dict[str, Any], + data_two: dict[str, Any], + expected_parameters: dict[str, Any], +) -> None: + # Setup the test + monkeypatch.chdir(tmp_path) + config_one = tmp_path / "config_one.yml" + config_one.write_text(yaml.dump(data_one)) + config_two = tmp_path / "config_two.yml" + config_two.write_text(yaml.dump(data_two)) + + # Invoke the command + runner = CliRunner() + with pytest.warns( + UserWarning, match="^Configuration files contain overlapping keys: {'seir'}.$" + ): + result = runner.invoke(patch, [config_one.name, config_two.name]) + assert result.exit_code == 0 + + # Check the output + patched_config = yaml.safe_load(result.output) + assert "seir" in patched_config + assert patched_config["seir"]["parameters"] == expected_parameters From a1042100ec28527d1e1ca04fcb61b0bee85825fe Mon Sep 17 00:00:00 2001 From: Timothy Willard <9395586+TimothyWillard@users.noreply.github.com> Date: Tue, 10 Dec 2024 15:48:18 -0500 Subject: [PATCH 05/18] Rename dup `get_logloss` to `simulate_proposal` --- flepimop/gempyor_pkg/src/gempyor/inference.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/flepimop/gempyor_pkg/src/gempyor/inference.py b/flepimop/gempyor_pkg/src/gempyor/inference.py index 525ce5cd6..8fb6ed900 100644 --- a/flepimop/gempyor_pkg/src/gempyor/inference.py +++ b/flepimop/gempyor_pkg/src/gempyor/inference.py @@ -438,7 +438,7 @@ def get_all_sim_arguments(self): self.save, ] - def get_logloss(self, proposal): + def simulate_proposal(self, proposal): if not self.inferpar.check_in_bound(proposal=proposal): if not self.silent: print("`llik` is -inf (out of bound proposal).") From 02678a30465e8409ba0cc21c6353acf440d44120 Mon Sep 17 00:00:00 2001 From: Timothy Willard <9395586+TimothyWillard@users.noreply.github.com> Date: Wed, 11 Dec 2024 08:30:05 -0500 Subject: [PATCH 06/18] Fix `parse_config_files` TL key override behavior Corrected an issue where `parse_config_files` would try to merge top-level keys which comes from `confuse`. Work around is to manually build a dictionary and then convert that to a `Configuration` object. --- flepimop/gempyor_pkg/src/gempyor/shared_cli.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/flepimop/gempyor_pkg/src/gempyor/shared_cli.py b/flepimop/gempyor_pkg/src/gempyor/shared_cli.py index 3e5f0daea..4c446d643 100644 --- a/flepimop/gempyor_pkg/src/gempyor/shared_cli.py +++ b/flepimop/gempyor_pkg/src/gempyor/shared_cli.py @@ -237,12 +237,15 @@ def _parse_option(param: click.Parameter, value: Any) -> Any: ) config_src = _parse_option(config_validator, kwargs[config_key]) cfg.clear() + cfg_data = {} for config_file in config_src: tmp = confuse.Configuration("tmp") tmp.set_file(config_file) - if intersect := set(tmp.keys()) & set(cfg.keys()): + if intersect := set(tmp.keys()) & set(cfg_data.keys()): warnings.warn(f"Configuration files contain overlapping keys: {intersect}.") - cfg.set_file(config_file) + for k in tmp.keys(): + cfg_data[k] = tmp[k].get() + cfg.set(cfg_data) cfg["config_src"] = [str(k) for k in config_src] # deal with the scenario overrides From fe1dc2b52d72c1084e87ad14b4a410f266eabc24 Mon Sep 17 00:00:00 2001 From: Timothy Willard <9395586+TimothyWillard@users.noreply.github.com> Date: Wed, 11 Dec 2024 08:58:07 -0500 Subject: [PATCH 07/18] Add `--indent` option to `flepimop patch` For configuration of the outputted yaml. --- flepimop/gempyor_pkg/src/gempyor/cli.py | 23 ++++++++++++++---- .../tests/cli/test_flepimop_patch_cli.py | 24 +++++++++++++++++++ 2 files changed, 42 insertions(+), 5 deletions(-) diff --git a/flepimop/gempyor_pkg/src/gempyor/cli.py b/flepimop/gempyor_pkg/src/gempyor/cli.py index 70da194d2..45433f4d5 100644 --- a/flepimop/gempyor_pkg/src/gempyor/cli.py +++ b/flepimop/gempyor_pkg/src/gempyor/cli.py @@ -1,4 +1,5 @@ -from click import pass_context, Context +import click +import yaml from .shared_cli import ( config_files_argument, @@ -20,12 +21,24 @@ # add some basic commands to the CLI -@cli.command(params=[config_files_argument] + list(config_file_options.values())) -@pass_context -def patch(ctx: Context = mock_context, **kwargs) -> None: +@cli.command( + params=[config_files_argument] + + list(config_file_options.values()) + + [ + click.Option( + ["--indent"], + type=click.IntRange(min=1), + required=False, + default=2, + help="Indentation level for the output YAML.", + ) + ], +) +@click.pass_context +def patch(ctx: click.Context = mock_context, **kwargs) -> None: """Merge configuration files""" parse_config_files(config, ctx, **kwargs) - print(config.dump()) + print(yaml.dump(yaml.safe_load(config.dump()), indent=kwargs["indent"])) if __name__ == "__main__": diff --git a/flepimop/gempyor_pkg/tests/cli/test_flepimop_patch_cli.py b/flepimop/gempyor_pkg/tests/cli/test_flepimop_patch_cli.py index 72d0134b9..800fac3b1 100644 --- a/flepimop/gempyor_pkg/tests/cli/test_flepimop_patch_cli.py +++ b/flepimop/gempyor_pkg/tests/cli/test_flepimop_patch_cli.py @@ -74,3 +74,27 @@ def test_patch_seir_parameters_behavior( patched_config = yaml.safe_load(result.output) assert "seir" in patched_config assert patched_config["seir"]["parameters"] == expected_parameters + + +@pytest.mark.parametrize("indent", (2, 4, 6)) +def test_user_provided_indent_size( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, indent: int +) -> None: + # Setup the test + monkeypatch.chdir(tmp_path) + config = tmp_path / "config.yml" + config.write_text(yaml.dump({"seir": {"parameters": {"beta": {"value": 1.2}}}})) + + # Invoke the command + runner = CliRunner() + result = runner.invoke(patch, [config.name, "--indent", str(indent)]) + assert result.exit_code == 0 + + # Check the output indentation, manually since `yaml.load` abstracts spacing away + for line in result.output.split(): + stripped_line = line.lstrip() + if stripped_line and not stripped_line.startswith("#"): + indent_size = len(line) - len(stripped_line) + if indent_size > 0: + assert indent_size == indent + break From ba22f0bf147b37f2fcb63e98523845c6119cb260 Mon Sep 17 00:00:00 2001 From: Timothy Willard <9395586+TimothyWillard@users.noreply.github.com> Date: Wed, 11 Dec 2024 10:16:31 -0500 Subject: [PATCH 08/18] Add detailed example to `flepimop patch --help` --- flepimop/gempyor_pkg/src/gempyor/cli.py | 80 ++++++++++++++++++++++++- 1 file changed, 79 insertions(+), 1 deletion(-) diff --git a/flepimop/gempyor_pkg/src/gempyor/cli.py b/flepimop/gempyor_pkg/src/gempyor/cli.py index 45433f4d5..bc71a8fe5 100644 --- a/flepimop/gempyor_pkg/src/gempyor/cli.py +++ b/flepimop/gempyor_pkg/src/gempyor/cli.py @@ -36,7 +36,85 @@ ) @click.pass_context def patch(ctx: click.Context = mock_context, **kwargs) -> None: - """Merge configuration files""" + """Merge configuration files + + This command will merge multiple config files together by overriding the top level + keys in config files. The order of the config files is important, as the last file + has the highest priority and the first has the lowest. + + A brief example: + + \b + ```bash + $ cd $(mktemp -d) + $ cat << EOF > config1.yml + compartments: + infection_stage: ['S', 'I', 'R'] + seir: + parameters: + beta: + value: 1.2 + EOF + $ cat << EOF > config2.yml + name: 'more parameters' + seir: + parameters: + beta: + value: 3.4 + gamma: + value: 5.6 + EOF + $ flepimop patch config1.yml config2.yml --indent 4 + ...: UserWarning: Configuration files contain overlapping keys: {'seir'}. + warnings.warn(f"Configuration files contain overlapping keys: {intersect}.") + compartments: + infection_stage: + - S + - I + - R + config_src: + - config1.yml + - config2.yml + first_sim_index: 1 + jobs: 14 + name: more parameters + outcome_modifiers_scenarios: [] + seir: + parameters: + beta: + value: 3.4 + gamma: + value: 5.6 + seir_modifiers_scenarios: [] + stoch_traj_flag: false + write_csv: false + write_parquet: true + $ flepimop patch config2.yml config1.yml --indent 4 + ...: UserWarning: Configuration files contain overlapping keys: {'seir'}. + warnings.warn(f"Configuration files contain overlapping keys: {intersect}.") + compartments: + infection_stage: + - S + - I + - R + config_src: + - config2.yml + - config1.yml + first_sim_index: 1 + jobs: 14 + name: more parameters + outcome_modifiers_scenarios: [] + seir: + parameters: + beta: + value: 1.2 + parameters: null + seir_modifiers_scenarios: [] + stoch_traj_flag: false + write_csv: false + write_parquet: true + ``` + """ parse_config_files(config, ctx, **kwargs) print(yaml.dump(yaml.safe_load(config.dump()), indent=kwargs["indent"])) From d3f89d222e1864d55554a292727ce941c965d41a Mon Sep 17 00:00:00 2001 From: Timothy Willard <9395586+TimothyWillard@users.noreply.github.com> Date: Wed, 11 Dec 2024 11:27:05 -0500 Subject: [PATCH 09/18] Force default indent of `flepimop patch` to 4 --- flepimop/gempyor_pkg/src/gempyor/cli.py | 20 ++++------------ .../tests/cli/test_flepimop_patch_cli.py | 24 ------------------- 2 files changed, 4 insertions(+), 40 deletions(-) diff --git a/flepimop/gempyor_pkg/src/gempyor/cli.py b/flepimop/gempyor_pkg/src/gempyor/cli.py index bc71a8fe5..43bad34fc 100644 --- a/flepimop/gempyor_pkg/src/gempyor/cli.py +++ b/flepimop/gempyor_pkg/src/gempyor/cli.py @@ -21,19 +21,7 @@ # add some basic commands to the CLI -@cli.command( - params=[config_files_argument] - + list(config_file_options.values()) - + [ - click.Option( - ["--indent"], - type=click.IntRange(min=1), - required=False, - default=2, - help="Indentation level for the output YAML.", - ) - ], -) +@cli.command(params=[config_files_argument] + list(config_file_options.values())) @click.pass_context def patch(ctx: click.Context = mock_context, **kwargs) -> None: """Merge configuration files @@ -64,7 +52,7 @@ def patch(ctx: click.Context = mock_context, **kwargs) -> None: gamma: value: 5.6 EOF - $ flepimop patch config1.yml config2.yml --indent 4 + $ flepimop patch config1.yml config2.yml ...: UserWarning: Configuration files contain overlapping keys: {'seir'}. warnings.warn(f"Configuration files contain overlapping keys: {intersect}.") compartments: @@ -89,7 +77,7 @@ def patch(ctx: click.Context = mock_context, **kwargs) -> None: stoch_traj_flag: false write_csv: false write_parquet: true - $ flepimop patch config2.yml config1.yml --indent 4 + $ flepimop patch config2.yml config1.yml ...: UserWarning: Configuration files contain overlapping keys: {'seir'}. warnings.warn(f"Configuration files contain overlapping keys: {intersect}.") compartments: @@ -116,7 +104,7 @@ def patch(ctx: click.Context = mock_context, **kwargs) -> None: ``` """ parse_config_files(config, ctx, **kwargs) - print(yaml.dump(yaml.safe_load(config.dump()), indent=kwargs["indent"])) + print(yaml.dump(yaml.safe_load(config.dump()), indent=4)) if __name__ == "__main__": diff --git a/flepimop/gempyor_pkg/tests/cli/test_flepimop_patch_cli.py b/flepimop/gempyor_pkg/tests/cli/test_flepimop_patch_cli.py index 800fac3b1..72d0134b9 100644 --- a/flepimop/gempyor_pkg/tests/cli/test_flepimop_patch_cli.py +++ b/flepimop/gempyor_pkg/tests/cli/test_flepimop_patch_cli.py @@ -74,27 +74,3 @@ def test_patch_seir_parameters_behavior( patched_config = yaml.safe_load(result.output) assert "seir" in patched_config assert patched_config["seir"]["parameters"] == expected_parameters - - -@pytest.mark.parametrize("indent", (2, 4, 6)) -def test_user_provided_indent_size( - tmp_path: Path, monkeypatch: pytest.MonkeyPatch, indent: int -) -> None: - # Setup the test - monkeypatch.chdir(tmp_path) - config = tmp_path / "config.yml" - config.write_text(yaml.dump({"seir": {"parameters": {"beta": {"value": 1.2}}}})) - - # Invoke the command - runner = CliRunner() - result = runner.invoke(patch, [config.name, "--indent", str(indent)]) - assert result.exit_code == 0 - - # Check the output indentation, manually since `yaml.load` abstracts spacing away - for line in result.output.split(): - stripped_line = line.lstrip() - if stripped_line and not stripped_line.startswith("#"): - indent_size = len(line) - len(stripped_line) - if indent_size > 0: - assert indent_size == indent - break From b5142d9ebac74109696cc42462f2488350c05e72 Mon Sep 17 00:00:00 2001 From: Timothy Willard <9395586+TimothyWillard@users.noreply.github.com> Date: Wed, 11 Dec 2024 11:45:20 -0500 Subject: [PATCH 10/18] Upgrade TL key collision from warning to error --- .../gempyor_pkg/src/gempyor/shared_cli.py | 4 +++- .../tests/cli/test_flepimop_patch_cli.py | 23 +++++++------------ .../shared_cli/test_parse_config_files.py | 2 +- 3 files changed, 12 insertions(+), 17 deletions(-) diff --git a/flepimop/gempyor_pkg/src/gempyor/shared_cli.py b/flepimop/gempyor_pkg/src/gempyor/shared_cli.py index 4c446d643..f98449e61 100644 --- a/flepimop/gempyor_pkg/src/gempyor/shared_cli.py +++ b/flepimop/gempyor_pkg/src/gempyor/shared_cli.py @@ -242,7 +242,9 @@ def _parse_option(param: click.Parameter, value: Any) -> Any: tmp = confuse.Configuration("tmp") tmp.set_file(config_file) if intersect := set(tmp.keys()) & set(cfg_data.keys()): - warnings.warn(f"Configuration files contain overlapping keys: {intersect}.") + raise ValueError( + f"Configuration files contain overlapping keys: {intersect}." + ) for k in tmp.keys(): cfg_data[k] = tmp[k].get() cfg.set(cfg_data) diff --git a/flepimop/gempyor_pkg/tests/cli/test_flepimop_patch_cli.py b/flepimop/gempyor_pkg/tests/cli/test_flepimop_patch_cli.py index 72d0134b9..053aed7f0 100644 --- a/flepimop/gempyor_pkg/tests/cli/test_flepimop_patch_cli.py +++ b/flepimop/gempyor_pkg/tests/cli/test_flepimop_patch_cli.py @@ -9,7 +9,7 @@ @pytest.mark.parametrize( - ("data_one", "data_two", "expected_parameters"), + ("data_one", "data_two"), ( ( { @@ -26,7 +26,6 @@ } } }, - {"gamma": {"value": 3.4}}, ), ( { @@ -44,16 +43,14 @@ } } }, - {"gamma": {"value": 3.4}}, ), ), ) -def test_patch_seir_parameters_behavior( +def test_overlapping_sections_value_error( tmp_path: Path, monkeypatch: pytest.MonkeyPatch, data_one: dict[str, Any], data_two: dict[str, Any], - expected_parameters: dict[str, Any], ) -> None: # Setup the test monkeypatch.chdir(tmp_path) @@ -64,13 +61,9 @@ def test_patch_seir_parameters_behavior( # Invoke the command runner = CliRunner() - with pytest.warns( - UserWarning, match="^Configuration files contain overlapping keys: {'seir'}.$" - ): - result = runner.invoke(patch, [config_one.name, config_two.name]) - assert result.exit_code == 0 - - # Check the output - patched_config = yaml.safe_load(result.output) - assert "seir" in patched_config - assert patched_config["seir"]["parameters"] == expected_parameters + result = runner.invoke(patch, [config_one.name, config_two.name]) + assert result.exit_code == 1 + assert isinstance(result.exception, ValueError) + assert ( + str(result.exception) == "Configuration files contain overlapping keys: {'seir'}." + ) diff --git a/flepimop/gempyor_pkg/tests/shared_cli/test_parse_config_files.py b/flepimop/gempyor_pkg/tests/shared_cli/test_parse_config_files.py index a64fc38f6..a2aa55c47 100644 --- a/flepimop/gempyor_pkg/tests/shared_cli/test_parse_config_files.py +++ b/flepimop/gempyor_pkg/tests/shared_cli/test_parse_config_files.py @@ -158,7 +158,7 @@ def test_multifile_config_collision( tmpconfigfile1 = config_file(tmp_path, testdict1, "config1.yaml") tmpconfigfile2 = config_file(tmp_path, testdict2, "config2.yaml") mockconfig = mock_empty_config() - with pytest.warns(UserWarning, match=r"foo"): + with pytest.raises(ValueError, match=r"foo"): parse_config_files(mockconfig, config_files=[tmpconfigfile1, tmpconfigfile2]) for k, v in (testdict1 | testdict2).items(): assert mockconfig[k].get(v) == v From b34792dede478e61f4e1c561fb98ed98c466d368 Mon Sep 17 00:00:00 2001 From: "Carl A. B. Pearson" Date: Wed, 11 Dec 2024 12:18:21 -0500 Subject: [PATCH 11/18] modify approach to handling help --- flepimop/gempyor_pkg/src/gempyor/cli.py | 5 ++++- flepimop/gempyor_pkg/src/gempyor/shared_cli.py | 4 +++- flepimop/gempyor_pkg/src/gempyor/simulate.py | 4 +++- 3 files changed, 10 insertions(+), 3 deletions(-) diff --git a/flepimop/gempyor_pkg/src/gempyor/cli.py b/flepimop/gempyor_pkg/src/gempyor/cli.py index 43bad34fc..ea67699bd 100644 --- a/flepimop/gempyor_pkg/src/gempyor/cli.py +++ b/flepimop/gempyor_pkg/src/gempyor/cli.py @@ -21,7 +21,10 @@ # add some basic commands to the CLI -@cli.command(params=[config_files_argument] + list(config_file_options.values())) +@cli.command( + params=[config_files_argument] + list(config_file_options.values()), + context_settings=dict(help_option_names=["-h", "--help"]), +) @click.pass_context def patch(ctx: click.Context = mock_context, **kwargs) -> None: """Merge configuration files diff --git a/flepimop/gempyor_pkg/src/gempyor/shared_cli.py b/flepimop/gempyor_pkg/src/gempyor/shared_cli.py index f98449e61..d73a2c5f1 100644 --- a/flepimop/gempyor_pkg/src/gempyor/shared_cli.py +++ b/flepimop/gempyor_pkg/src/gempyor/shared_cli.py @@ -222,7 +222,9 @@ def _parse_option(param: click.Parameter, value: Any) -> Any: config_src = [] if len(found_configs) != 1: if not found_configs: - raise ValueError(f"No config files provided.") + click.echo("No configuration provided! See help for required usage:\n") + click.echo(ctx.get_help()) + ctx.exit() else: error_dict = {k: kwargs[k] for k in found_configs} raise ValueError( diff --git a/flepimop/gempyor_pkg/src/gempyor/simulate.py b/flepimop/gempyor_pkg/src/gempyor/simulate.py index e8602818d..2b9ffda37 100644 --- a/flepimop/gempyor_pkg/src/gempyor/simulate.py +++ b/flepimop/gempyor_pkg/src/gempyor/simulate.py @@ -300,7 +300,9 @@ def simulate( @cli.command( - name="simulate", params=[config_files_argument] + list(config_file_options.values()) + name="simulate", + params=[config_files_argument] + list(config_file_options.values()), + context_settings=dict(help_option_names=["-h", "--help"]), ) @pass_context def _click_simulate(ctx: Context, **kwargs) -> int: From 7ae10375f1e6e28c0b2dd1fa20394a7f0a8d670a Mon Sep 17 00:00:00 2001 From: Timothy Willard <9395586+TimothyWillard@users.noreply.github.com> Date: Wed, 11 Dec 2024 13:08:45 -0500 Subject: [PATCH 12/18] Add `_dump_formatted_yaml` for custom yaml dump Added an internal utility to custom format the yaml representation of a `confuse.Configuration` object. Creates a custom `yaml.Dumper` class that contains all of the custom representation changes as to not modify the `yaml` pkg defaults. --- flepimop/gempyor_pkg/src/gempyor/cli.py | 4 +- flepimop/gempyor_pkg/src/gempyor/utils.py | 72 +++++++++++++++++++ .../tests/utils/test__dump_formatted_yaml.py | 55 ++++++++++++++ 3 files changed, 129 insertions(+), 2 deletions(-) create mode 100644 flepimop/gempyor_pkg/tests/utils/test__dump_formatted_yaml.py diff --git a/flepimop/gempyor_pkg/src/gempyor/cli.py b/flepimop/gempyor_pkg/src/gempyor/cli.py index ea67699bd..9e7dd657a 100644 --- a/flepimop/gempyor_pkg/src/gempyor/cli.py +++ b/flepimop/gempyor_pkg/src/gempyor/cli.py @@ -8,7 +8,7 @@ cli, mock_context, ) -from .utils import config +from .utils import _dump_formatted_yaml, config # register the commands from the other modules from . import compartments, simulate @@ -107,7 +107,7 @@ def patch(ctx: click.Context = mock_context, **kwargs) -> None: ``` """ parse_config_files(config, ctx, **kwargs) - print(yaml.dump(yaml.safe_load(config.dump()), indent=4)) + print(_dump_formatted_yaml(config)) if __name__ == "__main__": diff --git a/flepimop/gempyor_pkg/src/gempyor/utils.py b/flepimop/gempyor_pkg/src/gempyor/utils.py index 2a848d5b9..af4fbf950 100644 --- a/flepimop/gempyor_pkg/src/gempyor/utils.py +++ b/flepimop/gempyor_pkg/src/gempyor/utils.py @@ -17,6 +17,7 @@ import scipy.ndimage import scipy.stats import sympy.parsing.sympy_parser +import yaml from . import file_paths @@ -1039,3 +1040,74 @@ def move_file_at_local(name_map: dict[str, str]) -> None: for src, dst in name_map.items(): os.path.makedirs(os.path.dirname(dst), exist_ok=True) shutil.copy(src, dst) + + +def _dump_formatted_yaml(cfg: confuse.Configuration) -> str: + """ + Dump confuse configuration to a formatted YAML string. + + Args: + cfg: The confuse configuration object. + + Returns: + A formatted YAML string representation of the configuration. + + Examples: + >>> from gempyor.utils import _dump_formatted_yaml + >>> import confuse + >>> conf = confuse.Configuration("foobar") + >>> data = { + ... "name": "Test Config", + ... "compartments": { + ... "infection_stage": ["S", "E", "I", "R"] + ... }, + ... "seir": { + ... "parameters": { + ... "beta": {"value": 3.4}, + ... "gamma": {"value": 5.6}, + ... }, + ... "transitions": { + ... "source": ["S"], + ... "destination": ["E"], + ... "rate": ["beta * gamma"], + ... "proportional_to": [["S"], ["I"]], + ... "proportion_exponent": [1, 1], + ... }, + ... }, + ... } + >>> conf.set(data) + >>> print(_dump_formatted_yaml(conf)) + name: "Test Config" + compartments: + infection_stage: [S, E, I, R] + seir: + parameters: + beta: + value: 3.4 + gamma: + value: 5.6 + transitions: + source: [S] + destination: [E] + rate: ["beta * gamma"] + proportional_to: [[S], [I]] + proportion_exponent: [1, 1] + """ + + class CustomDumper(yaml.Dumper): + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + self.add_representer(list, self._represent_list) + self.add_representer(str, self._represent_str) + + def _represent_list(self, dumper, data): + return dumper.represent_sequence("tag:yaml.org,2002:seq", data, flow_style=True) + + def _represent_str(self, dumper, data): + if " " in data: + return dumper.represent_scalar("tag:yaml.org,2002:str", data, style='"') + return dumper.represent_scalar("tag:yaml.org,2002:str", data) + + return yaml.dump( + yaml.safe_load(cfg.dump()), Dumper=CustomDumper, indent=4, sort_keys=False + ) diff --git a/flepimop/gempyor_pkg/tests/utils/test__dump_formatted_yaml.py b/flepimop/gempyor_pkg/tests/utils/test__dump_formatted_yaml.py new file mode 100644 index 000000000..4ee574eea --- /dev/null +++ b/flepimop/gempyor_pkg/tests/utils/test__dump_formatted_yaml.py @@ -0,0 +1,55 @@ +from typing import Any + +import confuse +import pytest + +from gempyor.utils import _dump_formatted_yaml + + +@pytest.mark.parametrize( + ("data", "expected"), + ( + ({"key": "value"}, "key: value\n"), + ( + {"name": "Test Config", "compartments": {"infection_stage": ["S", "I", "R"]}}, + """name: "Test Config" +compartments: + infection_stage: [S, I, R] +""", + ), + ( + { + "seir": { + "parameters": { + "beta": {"value": 3.4}, + "gamma": {"value": 5.6}, + }, + "transitions": { + "source": ["S"], + "destination": ["E"], + "rate": ["beta * gamma"], + "proportional_to": [["S"], ["I"]], + "proportion_exponent": [1, 1], + }, + } + }, + """seir: + parameters: + beta: + value: 3.4 + gamma: + value: 5.6 + transitions: + source: [S] + destination: [E] + rate: ["beta * gamma"] + proportional_to: [[S], [I]] + proportion_exponent: [1, 1] +""", + ), + ), +) +def test_exact_output_for_select_values(data: dict[str, Any], expected: str) -> None: + cfg = confuse.Configuration("test", __name__) + cfg.set(data) + assert _dump_formatted_yaml(cfg) == expected From 0fe483ed0a43444e91afca6e10e1be10a948663b Mon Sep 17 00:00:00 2001 From: Timothy Willard <9395586+TimothyWillard@users.noreply.github.com> Date: Wed, 11 Dec 2024 13:20:08 -0500 Subject: [PATCH 13/18] `flepimop patch --help` uses example configs Changed the help page to use existing files in the `examples/tutorials` directory rather than construction test configs on the fly per a suggestion by @pearsonca. --- flepimop/gempyor_pkg/src/gempyor/cli.py | 129 ++++++++++++------------ 1 file changed, 66 insertions(+), 63 deletions(-) diff --git a/flepimop/gempyor_pkg/src/gempyor/cli.py b/flepimop/gempyor_pkg/src/gempyor/cli.py index 9e7dd657a..405509ba7 100644 --- a/flepimop/gempyor_pkg/src/gempyor/cli.py +++ b/flepimop/gempyor_pkg/src/gempyor/cli.py @@ -33,77 +33,80 @@ def patch(ctx: click.Context = mock_context, **kwargs) -> None: keys in config files. The order of the config files is important, as the last file has the highest priority and the first has the lowest. - A brief example: + A brief example of the command is shown below using the sample config files from the + `examples/tutorials` directory. The command will merge the two files together and + print the resulting configuration to the console. \b ```bash - $ cd $(mktemp -d) - $ cat << EOF > config1.yml - compartments: - infection_stage: ['S', 'I', 'R'] - seir: - parameters: - beta: - value: 1.2 - EOF - $ cat << EOF > config2.yml - name: 'more parameters' - seir: - parameters: - beta: - value: 3.4 - gamma: - value: 5.6 - EOF - $ flepimop patch config1.yml config2.yml - ...: UserWarning: Configuration files contain overlapping keys: {'seir'}. - warnings.warn(f"Configuration files contain overlapping keys: {intersect}.") - compartments: - infection_stage: - - S - - I - - R - config_src: - - config1.yml - - config2.yml - first_sim_index: 1 - jobs: 14 - name: more parameters + $ flepimop patch config_sample_2pop_modifiers_part.yml config_sample_2pop_outcomes_part.yml > config_sample_2pop_patched.yml + $ cat config_sample_2pop_patched.yml outcome_modifiers_scenarios: [] - seir: - parameters: - beta: - value: 3.4 - gamma: - value: 5.6 - seir_modifiers_scenarios: [] - stoch_traj_flag: false - write_csv: false - write_parquet: true - $ flepimop patch config2.yml config1.yml - ...: UserWarning: Configuration files contain overlapping keys: {'seir'}. - warnings.warn(f"Configuration files contain overlapping keys: {intersect}.") - compartments: - infection_stage: - - S - - I - - R - config_src: - - config2.yml - - config1.yml first_sim_index: 1 jobs: 14 - name: more parameters - outcome_modifiers_scenarios: [] - seir: - parameters: - beta: - value: 1.2 - parameters: null - seir_modifiers_scenarios: [] stoch_traj_flag: false - write_csv: false + seir_modifiers_scenarios: [] write_parquet: true + write_csv: false + config_src: [config_sample_2pop_modifiers_part.yml, config_sample_2pop_outcomes_part.yml] + seir_modifiers: + scenarios: [Ro_lockdown, Ro_all] + modifiers: + Ro_lockdown: + method: SinglePeriodModifier + parameter: Ro + period_start_date: 2020-03-15 + period_end_date: 2020-05-01 + subpop: all + value: 0.4 + Ro_relax: + method: SinglePeriodModifier + parameter: Ro + period_start_date: 2020-05-01 + period_end_date: 2020-08-31 + subpop: all + value: 0.8 + Ro_all: + method: StackedModifier + modifiers: [Ro_lockdown, Ro_relax] + outcome_modifiers: + scenarios: [test_limits] + modifiers: + test_limits: + method: SinglePeriodModifier + parameter: incidCase::probability + subpop: all + period_start_date: 2020-02-01 + period_end_date: 2020-06-01 + value: 0.5 + outcomes: + method: delayframe + outcomes: + incidCase: + source: + incidence: + infection_stage: I + probability: + value: 0.5 + delay: + value: 5 + incidHosp: + source: + incidence: + infection_stage: I + probability: + value: 0.05 + delay: + value: 7 + duration: + value: 10 + name: currHosp + incidDeath: + source: incidHosp + probability: + value: 0.2 + delay: + value: 14 ``` """ parse_config_files(config, ctx, **kwargs) From 719c23b1a47a37544de08179016b4f6671e1f011 Mon Sep 17 00:00:00 2001 From: "Carl A. B. Pearson" Date: Wed, 11 Dec 2024 13:47:20 -0500 Subject: [PATCH 14/18] update multi-configs gitbook --- .../gitbook/how-to-run/multi-configs.md | 37 ++----------------- 1 file changed, 4 insertions(+), 33 deletions(-) diff --git a/documentation/gitbook/how-to-run/multi-configs.md b/documentation/gitbook/how-to-run/multi-configs.md index 738de3399..27e65b324 100644 --- a/documentation/gitbook/how-to-run/multi-configs.md +++ b/documentation/gitbook/how-to-run/multi-configs.md @@ -32,46 +32,17 @@ you'll get a basic foward simulation of this example model. However, you might a flepimop simulate config_sample_2pop.yml config_sample_2pop_outcomes_part.yml ``` -if want to see what the combined configuration is, you can use the `patch` command: +if want to create a combined configuration (e.g. to check what the combined configuration is), you can use the `patch` command: ```bash -flepimop patch config_sample_2pop.yml config_sample_2pop_outcomes_part.yml +flepimop patch config_sample_2pop.yml config_sample_2pop_outcomes_part.yml > config_new.yml +cat config_new.yml ``` You may provide an arbitrary number of separate configuration files to combine to create a complete configuration. ## Caveats -At this time, only `simulate` supports multiple configuration files. Also, the patching operation is fairly crude: configuration options override previous ones completely, though with a warning. The files provided from left to right are from lowest priority (i.e. for the first file, only options specified in no other files are used) to highest priority (i.e. for the last file, its options override any other specification). +At this time, only `simulate` supports multiple configuration files. Also, the patching operation is fairly crude: if multiple configuration files specify the same option, this will yield an error. We are expanding coverage of this capability to other flepimop actions, e.g. inference, and are exploring options for smarter patching. - -However, currently there are pitfalls like - -```yaml -# config1 -seir_modifiers: - scenarios: ["one", "two"] - one: - # ... - two: - # ... -``` - -```yaml -# config2 -seir_modifiers: - scenarios: ["one", "three"] - one: - # ... - three: - # ... -``` - -Then you might expect - -```bash -flepimop simulate config1.yml config2.yml -``` - -...to override seir scenario one and add scenario three, but what actually happens is that the entire seir_modifiers from config1 is overriden by config2. Specifying the configuration files in the reverse order would lead to a different outcome (the config1 seir_modifiers overrides config2 settings). If you're doing complex combinations of configuration files, you should use `flepimop patch ...` to ensure you're getting what you expect. \ No newline at end of file From 11d8a2d233e8de500be78f72da5c9a989751284c Mon Sep 17 00:00:00 2001 From: Timothy Willard <9395586+TimothyWillard@users.noreply.github.com> Date: Wed, 11 Dec 2024 13:49:54 -0500 Subject: [PATCH 15/18] `parse_config_files` error includes config name --- flepimop/gempyor_pkg/src/gempyor/shared_cli.py | 4 +++- flepimop/gempyor_pkg/tests/cli/test_flepimop_patch_cli.py | 4 ++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/flepimop/gempyor_pkg/src/gempyor/shared_cli.py b/flepimop/gempyor_pkg/src/gempyor/shared_cli.py index d73a2c5f1..a88c599dd 100644 --- a/flepimop/gempyor_pkg/src/gempyor/shared_cli.py +++ b/flepimop/gempyor_pkg/src/gempyor/shared_cli.py @@ -244,8 +244,10 @@ def _parse_option(param: click.Parameter, value: Any) -> Any: tmp = confuse.Configuration("tmp") tmp.set_file(config_file) if intersect := set(tmp.keys()) & set(cfg_data.keys()): + intersect = ", ".join(sorted(list(intersect))) raise ValueError( - f"Configuration files contain overlapping keys: {intersect}." + "Configuration files contain overlapping keys, " + f"{intersect}, introduced by {config_file}." ) for k in tmp.keys(): cfg_data[k] = tmp[k].get() diff --git a/flepimop/gempyor_pkg/tests/cli/test_flepimop_patch_cli.py b/flepimop/gempyor_pkg/tests/cli/test_flepimop_patch_cli.py index 053aed7f0..35fa57470 100644 --- a/flepimop/gempyor_pkg/tests/cli/test_flepimop_patch_cli.py +++ b/flepimop/gempyor_pkg/tests/cli/test_flepimop_patch_cli.py @@ -64,6 +64,6 @@ def test_overlapping_sections_value_error( result = runner.invoke(patch, [config_one.name, config_two.name]) assert result.exit_code == 1 assert isinstance(result.exception, ValueError) - assert ( - str(result.exception) == "Configuration files contain overlapping keys: {'seir'}." + assert str(result.exception) == ( + "Configuration files contain overlapping keys, seir, introduced by config_two.yml." ) From ba9b5274d6d5c0dc5944bc4cdb987fb01a0c6274 Mon Sep 17 00:00:00 2001 From: Timothy Willard <9395586+TimothyWillard@users.noreply.github.com> Date: Wed, 11 Dec 2024 14:57:48 -0500 Subject: [PATCH 16/18] Resolve seir/outcome modifier scenarios --- flepimop/gempyor_pkg/src/gempyor/cli.py | 132 ++++--- .../gempyor_pkg/src/gempyor/shared_cli.py | 8 +- .../tests/cli/test_flepimop_patch_cli.py | 343 ++++++++++++++++++ 3 files changed, 413 insertions(+), 70 deletions(-) diff --git a/flepimop/gempyor_pkg/src/gempyor/cli.py b/flepimop/gempyor_pkg/src/gempyor/cli.py index 405509ba7..529fd2638 100644 --- a/flepimop/gempyor_pkg/src/gempyor/cli.py +++ b/flepimop/gempyor_pkg/src/gempyor/cli.py @@ -39,74 +39,72 @@ def patch(ctx: click.Context = mock_context, **kwargs) -> None: \b ```bash - $ flepimop patch config_sample_2pop_modifiers_part.yml config_sample_2pop_outcomes_part.yml > config_sample_2pop_patched.yml - $ cat config_sample_2pop_patched.yml - outcome_modifiers_scenarios: [] - first_sim_index: 1 - jobs: 14 - stoch_traj_flag: false - seir_modifiers_scenarios: [] - write_parquet: true - write_csv: false - config_src: [config_sample_2pop_modifiers_part.yml, config_sample_2pop_outcomes_part.yml] - seir_modifiers: - scenarios: [Ro_lockdown, Ro_all] - modifiers: - Ro_lockdown: - method: SinglePeriodModifier - parameter: Ro - period_start_date: 2020-03-15 - period_end_date: 2020-05-01 - subpop: all - value: 0.4 - Ro_relax: - method: SinglePeriodModifier - parameter: Ro - period_start_date: 2020-05-01 - period_end_date: 2020-08-31 - subpop: all - value: 0.8 - Ro_all: - method: StackedModifier - modifiers: [Ro_lockdown, Ro_relax] - outcome_modifiers: - scenarios: [test_limits] - modifiers: - test_limits: - method: SinglePeriodModifier - parameter: incidCase::probability - subpop: all - period_start_date: 2020-02-01 - period_end_date: 2020-06-01 - value: 0.5 - outcomes: - method: delayframe - outcomes: - incidCase: - source: - incidence: - infection_stage: I - probability: + $ flepimop patch config_sample_2pop_modifiers_part.yml config_sample_2pop_outcomes_part.yml > config_sample_2pop_patched.yml + $ cat config_sample_2pop_patched.yml + write_csv: false + stoch_traj_flag: false + jobs: 14 + write_parquet: true + first_sim_index: 1 + config_src: [config_sample_2pop_modifiers_part.yml, config_sample_2pop_outcomes_part.yml] + seir_modifiers: + scenarios: [Ro_lockdown, Ro_all] + modifiers: + Ro_lockdown: + method: SinglePeriodModifier + parameter: Ro + period_start_date: 2020-03-15 + period_end_date: 2020-05-01 + subpop: all + value: 0.4 + Ro_relax: + method: SinglePeriodModifier + parameter: Ro + period_start_date: 2020-05-01 + period_end_date: 2020-08-31 + subpop: all + value: 0.8 + Ro_all: + method: StackedModifier + modifiers: [Ro_lockdown, Ro_relax] + outcome_modifiers: + scenarios: [test_limits] + modifiers: + test_limits: + method: SinglePeriodModifier + parameter: incidCase::probability + subpop: all + period_start_date: 2020-02-01 + period_end_date: 2020-06-01 value: 0.5 - delay: - value: 5 - incidHosp: - source: - incidence: - infection_stage: I - probability: - value: 0.05 - delay: - value: 7 - duration: - value: 10 - name: currHosp - incidDeath: - source: incidHosp - probability: - value: 0.2 - delay: - value: 14 + outcomes: + method: delayframe + outcomes: + incidCase: + source: + incidence: + infection_stage: I + probability: + value: 0.5 + delay: + value: 5 + incidHosp: + source: + incidence: + infection_stage: I + probability: + value: 0.05 + delay: + value: 7 + duration: + value: 10 + name: currHosp + incidDeath: + source: incidHosp + probability: + value: 0.2 + delay: + value: 14 ``` """ parse_config_files(config, ctx, **kwargs) diff --git a/flepimop/gempyor_pkg/src/gempyor/shared_cli.py b/flepimop/gempyor_pkg/src/gempyor/shared_cli.py index a88c599dd..367769cde 100644 --- a/flepimop/gempyor_pkg/src/gempyor/shared_cli.py +++ b/flepimop/gempyor_pkg/src/gempyor/shared_cli.py @@ -255,12 +255,14 @@ def _parse_option(param: click.Parameter, value: Any) -> Any: cfg["config_src"] = [str(k) for k in config_src] # deal with the scenario overrides - scen_args = {k for k in parsed_args if k.endswith("scenarios") and kwargs.get(k)} - for option in scen_args: + scen_args = {k for k in parsed_args if k.endswith("_scenarios")} + for option in {s for s in scen_args if kwargs.get(s)}: key = option.replace("_scenarios", "") value = _parse_option(config_file_options[option], kwargs[option]) if cfg[key].exists(): - cfg[key]["scenarios"] = as_list(value) + cfg[key]["scenarios"] = ( + list(value) if isinstance(value, tuple) else as_list(value) + ) else: raise ValueError( f"Specified {option} when no {key} in configuration file(s): {config_src}" diff --git a/flepimop/gempyor_pkg/tests/cli/test_flepimop_patch_cli.py b/flepimop/gempyor_pkg/tests/cli/test_flepimop_patch_cli.py index 35fa57470..b4ba20021 100644 --- a/flepimop/gempyor_pkg/tests/cli/test_flepimop_patch_cli.py +++ b/flepimop/gempyor_pkg/tests/cli/test_flepimop_patch_cli.py @@ -67,3 +67,346 @@ def test_overlapping_sections_value_error( assert str(result.exception) == ( "Configuration files contain overlapping keys, seir, introduced by config_two.yml." ) + + +@pytest.mark.parametrize( + ("data", "seir_modifier_scenarios", "outcome_modifier_scenarios"), + ( + ( + { + "seir_modifiers": { + "scenarios": ["Ro_lockdown", "Ro_all"], + "modifiers": { + "Ro_lockdown": { + "method": "SinglePeriodModifier", + "parameter": "Ro", + "period_start_date": "2020-03-15", + "period_end_date": "2020-05-01", + "subpop": "all", + "value": 0.4, + }, + "Ro_relax": { + "method": "SinglePeriodModifier", + "parameter": "Ro", + "period_start_date": "2020-05-01", + "period_end_date": "2020-07-01", + "subpop": "all", + "value": 0.8, + }, + "Ro_all": { + "method": "StackedModifier", + "modifiers": ["Ro_lockdown", "Ro_relax"], + }, + }, + }, + }, + [], + [], + ), + ( + { + "seir_modifiers": { + "scenarios": ["Ro_lockdown", "Ro_all"], + "modifiers": { + "Ro_lockdown": { + "method": "SinglePeriodModifier", + "parameter": "Ro", + "period_start_date": "2020-03-15", + "period_end_date": "2020-05-01", + "subpop": "all", + "value": 0.4, + }, + "Ro_relax": { + "method": "SinglePeriodModifier", + "parameter": "Ro", + "period_start_date": "2020-05-01", + "period_end_date": "2020-07-01", + "subpop": "all", + "value": 0.8, + }, + "Ro_all": { + "method": "StackedModifier", + "modifiers": ["Ro_lockdown", "Ro_relax"], + }, + }, + }, + }, + ["Ro_all"], + [], + ), + ( + { + "seir_modifiers": { + "scenarios": ["Ro_lockdown", "Ro_all"], + "modifiers": { + "Ro_lockdown": { + "method": "SinglePeriodModifier", + "parameter": "Ro", + "period_start_date": "2020-03-15", + "period_end_date": "2020-05-01", + "subpop": "all", + "value": 0.4, + }, + "Ro_relax": { + "method": "SinglePeriodModifier", + "parameter": "Ro", + "period_start_date": "2020-05-01", + "period_end_date": "2020-07-01", + "subpop": "all", + "value": 0.8, + }, + "Ro_all": { + "method": "StackedModifier", + "modifiers": ["Ro_lockdown", "Ro_relax"], + }, + }, + }, + }, + ["Ro_all", "Ro_relax", "Ro_lockdown"], + [], + ), + ( + { + "outcome_modifiers": { + "scenarios": ["test_limits"], + "modifiers": { + "test_limits": { + "method": "SinglePeriodModifier", + "parameter": "incidCase::probability", + "subpop": "all", + "period_start_date": "2020-02-01", + "period_end_date": "2020-06-01", + "value": 0.5, + }, + "test_expansion": { + "method": "SinglePeriodModifier", + "parameter": "incidCase::probability", + "period_start_date": "2020-06-01", + "period_end_date": "2020-08-01", + "subpop": "all", + "value": 0.7, + }, + "test_limits_expansion": { + "method": "StackedModifier", + "modifiers": ["test_limits", "test_expansion"], + }, + }, + }, + }, + [], + [], + ), + ( + { + "outcome_modifiers": { + "scenarios": ["test_limits"], + "modifiers": { + "test_limits": { + "method": "SinglePeriodModifier", + "parameter": "incidCase::probability", + "subpop": "all", + "period_start_date": "2020-02-01", + "period_end_date": "2020-06-01", + "value": 0.5, + }, + "test_expansion": { + "method": "SinglePeriodModifier", + "parameter": "incidCase::probability", + "period_start_date": "2020-06-01", + "period_end_date": "2020-08-01", + "subpop": "all", + "value": 0.7, + }, + "test_limits_expansion": { + "method": "StackedModifier", + "modifiers": ["test_limits", "test_expansion"], + }, + }, + }, + }, + [], + ["test_limits_expansion"], + ), + ( + { + "outcome_modifiers": { + "scenarios": ["test_limits"], + "modifiers": { + "test_limits": { + "method": "SinglePeriodModifier", + "parameter": "incidCase::probability", + "subpop": "all", + "period_start_date": "2020-02-01", + "period_end_date": "2020-06-01", + "value": 0.5, + }, + "test_expansion": { + "method": "SinglePeriodModifier", + "parameter": "incidCase::probability", + "period_start_date": "2020-06-01", + "period_end_date": "2020-08-01", + "subpop": "all", + "value": 0.7, + }, + "test_limits_expansion": { + "method": "StackedModifier", + "modifiers": ["test_limits", "test_expansion"], + }, + }, + }, + }, + [], + ["test_limits", "test_expansion", "test_limits_expansion"], + ), + ( + { + "seir_modifiers": { + "scenarios": ["Ro_lockdown", "Ro_all"], + "modifiers": { + "Ro_lockdown": { + "method": "SinglePeriodModifier", + "parameter": "Ro", + "period_start_date": "2020-03-15", + "period_end_date": "2020-05-01", + "subpop": "all", + "value": 0.4, + }, + "Ro_relax": { + "method": "SinglePeriodModifier", + "parameter": "Ro", + "period_start_date": "2020-05-01", + "period_end_date": "2020-07-01", + "subpop": "all", + "value": 0.8, + }, + "Ro_all": { + "method": "StackedModifier", + "modifiers": ["Ro_lockdown", "Ro_relax"], + }, + }, + }, + "outcome_modifiers": { + "scenarios": ["test_limits"], + "modifiers": { + "test_limits": { + "method": "SinglePeriodModifier", + "parameter": "incidCase::probability", + "subpop": "all", + "period_start_date": "2020-02-01", + "period_end_date": "2020-06-01", + "value": 0.5, + }, + "test_expansion": { + "method": "SinglePeriodModifier", + "parameter": "incidCase::probability", + "period_start_date": "2020-06-01", + "period_end_date": "2020-08-01", + "subpop": "all", + "value": 0.7, + }, + "test_limits_expansion": { + "method": "StackedModifier", + "modifiers": ["test_limits", "test_expansion"], + }, + }, + }, + }, + [], + [], + ), + ( + { + "seir_modifiers": { + "scenarios": ["Ro_lockdown", "Ro_all"], + "modifiers": { + "Ro_lockdown": { + "method": "SinglePeriodModifier", + "parameter": "Ro", + "period_start_date": "2020-03-15", + "period_end_date": "2020-05-01", + "subpop": "all", + "value": 0.4, + }, + "Ro_relax": { + "method": "SinglePeriodModifier", + "parameter": "Ro", + "period_start_date": "2020-05-01", + "period_end_date": "2020-07-01", + "subpop": "all", + "value": 0.8, + }, + "Ro_all": { + "method": "StackedModifier", + "modifiers": ["Ro_lockdown", "Ro_relax"], + }, + }, + }, + "outcome_modifiers": { + "scenarios": ["test_limits"], + "modifiers": { + "test_limits": { + "method": "SinglePeriodModifier", + "parameter": "incidCase::probability", + "subpop": "all", + "period_start_date": "2020-02-01", + "period_end_date": "2020-06-01", + "value": 0.5, + }, + "test_expansion": { + "method": "SinglePeriodModifier", + "parameter": "incidCase::probability", + "period_start_date": "2020-06-01", + "period_end_date": "2020-08-01", + "subpop": "all", + "value": 0.7, + }, + "test_limits_expansion": { + "method": "StackedModifier", + "modifiers": ["test_limits", "test_expansion"], + }, + }, + }, + }, + ["Ro_relax"], + ["test_expansion"], + ), + ), +) +def test_editing_modifier_scenarios( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + data: dict[str, Any], + seir_modifier_scenarios: list[str], + outcome_modifier_scenarios: list[str], +) -> None: + # Setup the test + monkeypatch.chdir(tmp_path) + config_path = tmp_path / "config.yml" + config_path.write_text(yaml.dump(data)) + + # Invoke the command + runner = CliRunner() + args = [config_path.name] + if seir_modifier_scenarios: + for s in seir_modifier_scenarios: + args += ["--seir_modifiers_scenarios", s] + if outcome_modifier_scenarios: + for o in outcome_modifier_scenarios: + args += ["--outcome_modifiers_scenarios", o] + result = runner.invoke(patch, args) + assert result.exit_code == 0 + + # Check the output + patched_data = yaml.safe_load(result.output) + assert "seir_modifiers_scenarios" not in patched_data + assert patched_data.get("seir_modifiers", {}).get("scenarios", []) == ( + seir_modifier_scenarios + if seir_modifier_scenarios + else data.get("seir_modifiers", {}).get("scenarios", []) + ) + assert "outcome_modifiers_scenarios" not in patched_data + assert patched_data.get("outcome_modifiers", {}).get("scenarios", []) == ( + outcome_modifier_scenarios + if outcome_modifier_scenarios + else data.get("outcome_modifiers", {}).get("scenarios", []) + ) From c88c5240b3388198f838dd553cc8d06223f0e35a Mon Sep 17 00:00:00 2001 From: Timothy Willard <9395586+TimothyWillard@users.noreply.github.com> Date: Wed, 11 Dec 2024 15:39:05 -0500 Subject: [PATCH 17/18] `flepimop patch` documenation edits Stronger and clearer wording suggested by @saraloo. --- documentation/gitbook/how-to-run/multi-configs.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/documentation/gitbook/how-to-run/multi-configs.md b/documentation/gitbook/how-to-run/multi-configs.md index 27e65b324..54402c3b5 100644 --- a/documentation/gitbook/how-to-run/multi-configs.md +++ b/documentation/gitbook/how-to-run/multi-configs.md @@ -20,19 +20,19 @@ You should see an assortment of yml files as a result of that `ls` command. ## Usage -If you run +If you run: ```bash flepimop simulate config_sample_2pop.yml ``` -you'll get a basic foward simulation of this example model. However, you might also note there are several `*_part.yml` files, corresponding to partial configs. You can `simulate` using the combination of multiple configs with, for example: +You'll get a basic forward simulation of this example model. However, you might also note there are several `*_part.yml` files, corresponding to partial configs. You can `simulate` using the combination of multiple configs with, for example: ```bash flepimop simulate config_sample_2pop.yml config_sample_2pop_outcomes_part.yml ``` -if want to create a combined configuration (e.g. to check what the combined configuration is), you can use the `patch` command: +While simulate can run your patched configuration, we also suggest you check your configuration file using the patch command: ```bash flepimop patch config_sample_2pop.yml config_sample_2pop_outcomes_part.yml > config_new.yml @@ -43,6 +43,6 @@ You may provide an arbitrary number of separate configuration files to combine t ## Caveats -At this time, only `simulate` supports multiple configuration files. Also, the patching operation is fairly crude: if multiple configuration files specify the same option, this will yield an error. +At this time, only simulate directly supports multiple configuration files, and our current patching capabilities only allow for the addition of new sections as given in our tutorials. This is helpful for building models piece-by-piece from a simple compartmental forward simulation, to including outcome probabilities, and finally, adding modifier sections. If multiple configuration files specify the same higher level configuration chunks (e.g., seir, outcomes), this will yield an error. We are expanding coverage of this capability to other flepimop actions, e.g. inference, and are exploring options for smarter patching. From 4ecb2be97584b8496a7acc77b8548d20a2b487e5 Mon Sep 17 00:00:00 2001 From: Timothy Willard <9395586+TimothyWillard@users.noreply.github.com> Date: Thu, 12 Dec 2024 09:19:51 -0500 Subject: [PATCH 18/18] Add explicit types to `pull_request` trigger Includes the defaults as well as the 'edited' and `ready_for_review` types. --- .github/workflows/conda-env.yml | 6 ++++++ .github/workflows/flepicommon-ci.yml | 6 ++++++ .github/workflows/gempyor-ci.yml | 6 ++++++ .github/workflows/inference-ci.yml | 6 ++++++ .github/workflows/lint.yml | 6 ++++++ 5 files changed, 30 insertions(+) diff --git a/.github/workflows/conda-env.yml b/.github/workflows/conda-env.yml index 20ca91ade..711cd0968 100644 --- a/.github/workflows/conda-env.yml +++ b/.github/workflows/conda-env.yml @@ -9,6 +9,12 @@ on: branches: - dev pull_request: + types: + - edited + - opened + - ready_for_review + - reopened + - synchronize paths: - build/create_environment_yml.R - flepimop/R_packages/*/DESCRIPTION diff --git a/.github/workflows/flepicommon-ci.yml b/.github/workflows/flepicommon-ci.yml index 9610a88de..67888692f 100644 --- a/.github/workflows/flepicommon-ci.yml +++ b/.github/workflows/flepicommon-ci.yml @@ -8,6 +8,12 @@ on: branches: - dev pull_request: + types: + - edited + - opened + - ready_for_review + - reopened + - synchronize paths: - flepimop/R_packages/flepicommon/**/* branches: diff --git a/.github/workflows/gempyor-ci.yml b/.github/workflows/gempyor-ci.yml index d70d19071..97b2b2739 100644 --- a/.github/workflows/gempyor-ci.yml +++ b/.github/workflows/gempyor-ci.yml @@ -9,6 +9,12 @@ on: branches: - dev pull_request: + types: + - edited + - opened + - ready_for_review + - reopened + - synchronize paths: - examples/**/* - flepimop/gempyor_pkg/**/* diff --git a/.github/workflows/inference-ci.yml b/.github/workflows/inference-ci.yml index 31e561121..ffb503ab1 100644 --- a/.github/workflows/inference-ci.yml +++ b/.github/workflows/inference-ci.yml @@ -12,6 +12,12 @@ on: branches: - dev pull_request: + types: + - edited + - opened + - ready_for_review + - reopened + - synchronize paths: - flepimop/R_packages/inference/**/* branches: diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index e2449af27..790467d1b 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -6,6 +6,12 @@ on: paths: - 'flepimop/gempyor_pkg/**/*.py' pull_request: + types: + - edited + - opened + - ready_for_review + - reopened + - synchronize paths: - '**/*.py' branches: