From ad96e6f70588baec4e8246ff840839fcf7726770 Mon Sep 17 00:00:00 2001 From: s-weigand Date: Sat, 17 Dec 2022 20:19:35 +0100 Subject: [PATCH 1/6] =?UTF-8?q?=E2=9C=A8=20Added=20chdir=5Fcontext=20conte?= =?UTF-8?q?xt=20manager=20for=20testing=20with=20relative=20paths?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- glotaran/utils/io.py | 33 +++++++++++++++++++++++++++++ glotaran/utils/test/test_io.py | 38 ++++++++++++++++++++++++++++++++-- 2 files changed, 69 insertions(+), 2 deletions(-) diff --git a/glotaran/utils/io.py b/glotaran/utils/io.py index 765e8e3cd..43eee9f92 100644 --- a/glotaran/utils/io.py +++ b/glotaran/utils/io.py @@ -9,6 +9,7 @@ from collections.abc import Mapping from collections.abc import MutableMapping from collections.abc import Sequence +from contextlib import contextmanager from pathlib import Path from typing import TYPE_CHECKING from typing import Any @@ -19,6 +20,7 @@ from glotaran.typing.types import DatasetMappable if TYPE_CHECKING: + from collections.abc import Generator from collections.abc import Iterator import pandas as pd @@ -190,6 +192,37 @@ def load_datasets(dataset_mappable: DatasetMappable) -> DatasetMapping: return DatasetMapping.loader(dataset_mappable) +@contextmanager +def chdir_context(folder_path: StrOrPath) -> Generator[Path, None, None]: + """Context manager to change directory to ``folder_path``. + + Parameters + ---------- + folder_path: StrOrPath + Path to change to. + + Yields + ------ + Generator[Path, None, None] + Resolved path of ``folder_path``. + + Raises + ------ + ValueError + If ``folder_path`` is an existing file. + """ + original_dir = Path(os.curdir).resolve() + folder_path = Path(folder_path) + if folder_path.is_file() is True: + raise ValueError("Value of 'folder_path' needs to be a folder but was an existing file.") + folder_path.mkdir(parents=True, exist_ok=True) + try: + os.chdir(folder_path) + yield folder_path.resolve() + finally: + os.chdir(original_dir) + + def relative_posix_path(source_path: StrOrPath, base_path: StrOrPath | None = None) -> str: """Ensure that ``source_path`` is a posix path, relative to ``base_path`` if defined. diff --git a/glotaran/utils/test/test_io.py b/glotaran/utils/test/test_io.py index d03d7ab1b..7c0eb5212 100644 --- a/glotaran/utils/test/test_io.py +++ b/glotaran/utils/test/test_io.py @@ -2,6 +2,7 @@ from __future__ import annotations import html +import os import sys from pathlib import Path @@ -19,6 +20,7 @@ from glotaran.testing.simulated_data.sequential_spectral_decay import SCHEME from glotaran.testing.simulated_data.shared_decay import SPECTRAL_AXIS from glotaran.utils.io import DatasetMapping +from glotaran.utils.io import chdir_context from glotaran.utils.io import create_clp_guide_dataset from glotaran.utils.io import load_datasets from glotaran.utils.io import relative_posix_path @@ -196,11 +198,43 @@ def test_relative_posix_path(tmp_path: Path, rel_file_path: str): assert rel_result_path == rel_file_path - rel_result_no_coomon = relative_posix_path( + rel_result_no_common = relative_posix_path( (tmp_path / f"../{rel_file_path}").resolve().as_posix(), str(tmp_path) ) - assert rel_result_no_coomon == f"../{rel_file_path}" + assert rel_result_no_common == f"../{rel_file_path}" + + +def test_chdir_context(tmp_path: Path): + """Original Path is restored even after exception is thrown.""" + original_dir = Path(os.curdir).resolve() + with chdir_context(tmp_path) as curdir: + assert curdir == tmp_path.resolve() + assert tmp_path.resolve() == Path(os.curdir).resolve() + assert Path("test.txt").resolve() == (tmp_path / "test.txt").resolve() + + assert Path(os.curdir).resolve() == original_dir + + with pytest.raises(ValueError): + with chdir_context(tmp_path): + raise ValueError("Original path will be restored after I raise.") + + assert Path(os.curdir).resolve() == original_dir + + +def test_chdir_context_exception(tmp_path: Path): + """Raise error if ``folder_path`` is an existing file instead of a folder.""" + file_path = tmp_path / "test.txt" + file_path.touch() + + with pytest.raises(ValueError) as excinfo: + with chdir_context(file_path): + pass + + assert ( + str(excinfo.value) + == "Value of 'folder_path' needs to be a folder but was an existing file." + ) @pytest.mark.skipif(not sys.platform.startswith("win32"), reason="Only needed for Windows") From e5f083bd376cc5667cebbddcd8853479ff6f9634 Mon Sep 17 00:00:00 2001 From: s-weigand Date: Sat, 17 Dec 2022 22:57:57 +0100 Subject: [PATCH 2/6] =?UTF-8?q?=F0=9F=A7=AA=20Added=20failing=20test=20to?= =?UTF-8?q?=20reproduce=20save=5Fresult=20issue=20with=20relative=20paths?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../builtin/io/yml/test/test_save_result.py | 107 +++++++++++------- .../builtin/io/yml/test/test_save_scheme.py | 34 +++--- 2 files changed, 86 insertions(+), 55 deletions(-) diff --git a/glotaran/builtin/io/yml/test/test_save_result.py b/glotaran/builtin/io/yml/test/test_save_result.py index 45dec2616..df124a92c 100644 --- a/glotaran/builtin/io/yml/test/test_save_result.py +++ b/glotaran/builtin/io/yml/test/test_save_result.py @@ -13,6 +13,7 @@ from glotaran.optimization.optimize import optimize from glotaran.project.result import Result from glotaran.testing.simulated_data.sequential_spectral_decay import SCHEME +from glotaran.utils.io import chdir_context @pytest.fixture(scope="session") @@ -23,12 +24,10 @@ def dummy_result(): yield optimize(scheme, raise_exception=True) -def test_save_result_yml( - tmp_path: Path, - dummy_result: Result, -): +@pytest.mark.parametrize("path_is_absolute", (True, False)) +def test_save_result_yml(tmp_path: Path, dummy_result: Result, path_is_absolute: bool): """Check all files exist.""" - expected = dedent( + expected_result = dedent( f"""\ number_of_function_evaluations: 1 success: true @@ -49,49 +48,77 @@ def test_save_result_yml( dataset_1: dataset_1.nc """ ) + expected_scheme = dedent( + """\ + model: model.yml + parameters: initial_parameters.csv + data: + dataset_1: dataset_1.nc + clp_link_tolerance: 0.0 + clp_link_method: nearest + maximum_number_function_evaluations: 1 + add_svd: true + ftol: 1e-08 + gtol: 1e-08 + xtol: 1e-08 + optimization_method: TrustRegionReflection + result_path: null + """ + ) + if path_is_absolute is True: + result_dir = tmp_path / "testresult" + else: + result_dir = Path("testresult") - result_dir = tmp_path / "testresult" result_path = result_dir / "result.yml" - save_result(result_path=result_path, result=dummy_result) + with chdir_context("." if path_is_absolute is True else tmp_path): + save_result(result_path=result_path, result=dummy_result) - assert dummy_result.source_path == result_path.as_posix() + assert dummy_result.source_path == result_path.as_posix() - assert (result_dir / "result.md").exists() - assert (result_dir / "scheme.yml").exists() - assert result_path.exists() - assert (result_dir / "initial_parameters.csv").exists() - assert (result_dir / "optimized_parameters.csv").exists() - assert (result_dir / "optimization_history.csv").exists() - assert (result_dir / "dataset_1.nc").exists() + assert (result_dir / "result.md").exists() + assert (result_dir / "scheme.yml").exists() + assert (result_dir / "scheme.yml").read_text() == expected_scheme + assert result_path.exists() + assert (result_dir / "initial_parameters.csv").exists() + assert (result_dir / "optimized_parameters.csv").exists() + assert (result_dir / "optimization_history.csv").exists() + assert (result_dir / "dataset_1.nc").exists() - # We can't check equality due to numerical fluctuations - got = result_path.read_text() - print(got) - assert expected in got + # We can't check equality due to numerical fluctuations + got = result_path.read_text() + print(got) + assert expected_result in got -def test_save_result_yml_roundtrip(tmp_path: Path, dummy_result: Result): +@pytest.mark.parametrize("path_is_absolute", (True, False)) +def test_save_result_yml_roundtrip(tmp_path: Path, dummy_result: Result, path_is_absolute: bool): """Save and reloaded Result should be the same.""" - result_dir = tmp_path / "testresult" + if path_is_absolute is True: + result_dir = tmp_path / "testresult" + else: + result_dir = Path("testresult") result_path = result_dir / "result.yml" - save_result(result_path=result_path, result=dummy_result) - result_round_tripped = load_result(result_path) - assert dummy_result.source_path == result_path.as_posix() - assert result_round_tripped.source_path == result_path.as_posix() + with chdir_context("." if path_is_absolute is True else tmp_path): + save_result(result_path=result_path, result=dummy_result) + result_round_tripped = load_result(result_path) - assert_frame_equal( - dummy_result.initial_parameters.to_dataframe(), - result_round_tripped.initial_parameters.to_dataframe(), - ) - assert_frame_equal( - dummy_result.optimized_parameters.to_dataframe(), - result_round_tripped.optimized_parameters.to_dataframe(), - ) - assert_frame_equal( - dummy_result.parameter_history.to_dataframe(), - result_round_tripped.parameter_history.to_dataframe(), - ) - assert_frame_equal( - dummy_result.optimization_history.data, result_round_tripped.optimization_history.data - ) + assert dummy_result.source_path == result_path.as_posix() + assert result_round_tripped.source_path == result_path.as_posix() + + assert_frame_equal( + dummy_result.initial_parameters.to_dataframe(), + result_round_tripped.initial_parameters.to_dataframe(), + ) + assert_frame_equal( + dummy_result.optimized_parameters.to_dataframe(), + result_round_tripped.optimized_parameters.to_dataframe(), + ) + assert_frame_equal( + dummy_result.parameter_history.to_dataframe(), + result_round_tripped.parameter_history.to_dataframe(), + ) + assert_frame_equal( + dummy_result.optimization_history.data, result_round_tripped.optimization_history.data + ) diff --git a/glotaran/builtin/io/yml/test/test_save_scheme.py b/glotaran/builtin/io/yml/test/test_save_scheme.py index 7e2c2e74d..8a1e46cdb 100644 --- a/glotaran/builtin/io/yml/test/test_save_scheme.py +++ b/glotaran/builtin/io/yml/test/test_save_scheme.py @@ -1,7 +1,8 @@ from __future__ import annotations -from typing import TYPE_CHECKING +from pathlib import Path +import pytest import xarray as xr from glotaran.io import load_scheme @@ -13,10 +14,7 @@ from glotaran.testing.simulated_data.sequential_spectral_decay import DATASET from glotaran.testing.simulated_data.sequential_spectral_decay import MODEL from glotaran.testing.simulated_data.sequential_spectral_decay import PARAMETERS - -if TYPE_CHECKING: - from pathlib import Path - +from glotaran.utils.io import chdir_context want = """\ model: m.yml @@ -35,7 +33,8 @@ """ -def test_save_scheme(tmp_path: Path): +@pytest.mark.parametrize("path_is_absolute", (True, False)) +def test_save_scheme(tmp_path: Path, path_is_absolute: bool): save_model(MODEL, tmp_path / "m.yml") save_parameters(PARAMETERS, tmp_path / "p.csv") save_dataset(DATASET, tmp_path / "d.nc") @@ -44,12 +43,17 @@ def test_save_scheme(tmp_path: Path): PARAMETERS, {"dataset_1": DATASET}, ) - scheme_path = tmp_path / "testscheme.yml" - save_scheme(file_name=scheme_path, format_name="yml", scheme=scheme) - - assert scheme_path.is_file() - assert scheme_path.read_text() == want - loaded = load_scheme(scheme_path) - print(loaded.model.validate(loaded.parameters)) - assert loaded.model.valid(loaded.parameters) - assert isinstance(scheme.data["dataset_1"], xr.Dataset) + if path_is_absolute is True: + scheme_path = tmp_path / "testscheme.yml" + else: + scheme_path = Path("testscheme.yml") + + with chdir_context("." if path_is_absolute is True else tmp_path): + save_scheme(file_name=scheme_path, format_name="yml", scheme=scheme) + + assert scheme_path.is_file() + assert scheme_path.read_text() == want + loaded = load_scheme(scheme_path) + print(loaded.model.validate(loaded.parameters)) + assert loaded.model.valid(loaded.parameters) + assert isinstance(scheme.data["dataset_1"], xr.Dataset) From 2f1643b41fe7ac6515b86603721fc549aa4f0648 Mon Sep 17 00:00:00 2001 From: s-weigand Date: Sat, 17 Dec 2022 20:26:10 +0100 Subject: [PATCH 3/6] =?UTF-8?q?=F0=9F=A9=B9=20Fix=20wrong=20paths=20in=20r?= =?UTF-8?q?esult.yml=20when=20using=20relative=20path?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- glotaran/utils/io.py | 13 +++++++++---- glotaran/utils/test/test_io.py | 8 ++++++++ 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/glotaran/utils/io.py b/glotaran/utils/io.py index 43eee9f92..83e674cf6 100644 --- a/glotaran/utils/io.py +++ b/glotaran/utils/io.py @@ -226,6 +226,8 @@ def chdir_context(folder_path: StrOrPath) -> Generator[Path, None, None]: def relative_posix_path(source_path: StrOrPath, base_path: StrOrPath | None = None) -> str: """Ensure that ``source_path`` is a posix path, relative to ``base_path`` if defined. + For ``source_path`` to be converted to a relative path it either needs to a an absolute path or + ``base_path`` needs to be a parent directory of ``source_path``. On Windows if ``source_path`` and ``base_path`` are on different drives, it will return the absolute posix path to the file. @@ -234,17 +236,20 @@ def relative_posix_path(source_path: StrOrPath, base_path: StrOrPath | None = No source_path : StrOrPath Path which should be converted to a relative posix path. base_path : StrOrPath, optional - Base path the resulting path string should be relative to., by default None + Base path the resulting path string should be relative to. Defaults to ``None``. Returns ------- str ``source_path`` as posix path relative to ``base_path`` if defined. """ - source_path = Path(source_path).as_posix() - if base_path is not None and os.path.isabs(source_path): + source_path = Path(source_path) + if base_path is not None and ( + source_path.is_absolute() or Path(base_path).resolve() in source_path.resolve().parents + ): with contextlib.suppress(ValueError): - source_path = os.path.relpath(source_path, Path(base_path).as_posix()) + source_path = os.path.relpath(source_path.as_posix(), Path(base_path).as_posix()) + return Path(source_path).as_posix() diff --git a/glotaran/utils/test/test_io.py b/glotaran/utils/test/test_io.py index 7c0eb5212..d0c5c103f 100644 --- a/glotaran/utils/test/test_io.py +++ b/glotaran/utils/test/test_io.py @@ -204,6 +204,14 @@ def test_relative_posix_path(tmp_path: Path, rel_file_path: str): assert rel_result_no_common == f"../{rel_file_path}" + result_folder = tmp_path / "results" + with chdir_context(result_folder): + original_rel_path = relative_posix_path(rel_file_path, result_folder) + assert original_rel_path == rel_file_path + + original_rel_path = relative_posix_path(rel_file_path, "not_a_parent") + assert original_rel_path == rel_file_path + def test_chdir_context(tmp_path: Path): """Original Path is restored even after exception is thrown.""" From 12c0ea06c48b8fc22c946c17e68c5d231b45cdb2 Mon Sep 17 00:00:00 2001 From: s-weigand Date: Sun, 18 Dec 2022 18:00:40 +0100 Subject: [PATCH 4/6] =?UTF-8?q?=F0=9F=9A=A7=F0=9F=93=9A=20Added=20change?= =?UTF-8?q?=20to=20changelog?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- changelog.md | 1 + 1 file changed, 1 insertion(+) diff --git a/changelog.md b/changelog.md index 97d42c2ce..747430dbb 100644 --- a/changelog.md +++ b/changelog.md @@ -25,6 +25,7 @@ - 🩹 Fix for normalization issue described in #1157 (multi-gaussian irfs and multiple time ranges (streak)) - 🩹 Fix for crash described in #1183 when doing an optimization using more than 30 datasets (#1184) - 🩹 Fix pretty_format_numerical for negative values (#1192) +- 🩹 Fix yaml result saving with relative paths (#1199) From 3cb18c46af7a05f448b507b095746381ef1e0176 Mon Sep 17 00:00:00 2001 From: s-weigand Date: Sun, 18 Dec 2022 19:35:30 +0100 Subject: [PATCH 5/6] =?UTF-8?q?=F0=9F=A7=AA=20Added=20test=20where=20schem?= =?UTF-8?q?e=20data=20input=20path=20is=20different=20than=20save=20path?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../builtin/io/yml/test/test_save_result.py | 27 +++++++++++++++---- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/glotaran/builtin/io/yml/test/test_save_result.py b/glotaran/builtin/io/yml/test/test_save_result.py index df124a92c..0b5b9ada4 100644 --- a/glotaran/builtin/io/yml/test/test_save_result.py +++ b/glotaran/builtin/io/yml/test/test_save_result.py @@ -1,6 +1,7 @@ from __future__ import annotations from dataclasses import replace +from functools import lru_cache from pathlib import Path from textwrap import dedent @@ -9,6 +10,7 @@ from glotaran import __version__ from glotaran.io import load_result +from glotaran.io import save_dataset from glotaran.io import save_result from glotaran.optimization.optimize import optimize from glotaran.project.result import Result @@ -16,12 +18,18 @@ from glotaran.utils.io import chdir_context -@pytest.fixture(scope="session") -def dummy_result(): +@pytest.fixture +def dummy_result(tmp_path: Path): """Dummy result for testing.""" - print(SCHEME.data["dataset_1"]) - scheme = replace(SCHEME, maximum_number_function_evaluations=1) - yield optimize(scheme, raise_exception=True) + + @lru_cache + def create_result(): + scheme = replace(SCHEME, maximum_number_function_evaluations=1) + (tmp_path / "data").mkdir(parents=True, exist_ok=True) + save_dataset(scheme.data["dataset_1"], tmp_path / "data/ds1.nc") + return optimize(scheme, raise_exception=True) + + yield create_result() @pytest.mark.parametrize("path_is_absolute", (True, False)) @@ -70,6 +78,10 @@ def test_save_result_yml(tmp_path: Path, dummy_result: Result, path_is_absolute: else: result_dir = Path("testresult") + assert ( + dummy_result.scheme.data["dataset_1"].source_path == (tmp_path / "data/ds1.nc").as_posix() + ) + result_path = result_dir / "result.yml" with chdir_context("." if path_is_absolute is True else tmp_path): save_result(result_path=result_path, result=dummy_result) @@ -84,6 +96,11 @@ def test_save_result_yml(tmp_path: Path, dummy_result: Result, path_is_absolute: assert (result_dir / "optimized_parameters.csv").exists() assert (result_dir / "optimization_history.csv").exists() assert (result_dir / "dataset_1.nc").exists() + # Original scheme object isn't changed + assert ( + dummy_result.scheme.data["dataset_1"].source_path + == (tmp_path / "data/ds1.nc").as_posix() + ) # We can't check equality due to numerical fluctuations got = result_path.read_text() From 3d0dec5594ce4b67dbe624c8f404041c8e2fb3d7 Mon Sep 17 00:00:00 2001 From: s-weigand Date: Sun, 18 Dec 2022 19:52:29 +0100 Subject: [PATCH 6/6] =?UTF-8?q?=F0=9F=A9=B9=20Fixed=20dataset=20path=20in?= =?UTF-8?q?=20saved=20scheme?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- glotaran/builtin/io/yml/yml.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/glotaran/builtin/io/yml/yml.py b/glotaran/builtin/io/yml/yml.py index 24812cbc9..1a60318ff 100644 --- a/glotaran/builtin/io/yml/yml.py +++ b/glotaran/builtin/io/yml/yml.py @@ -1,6 +1,7 @@ """Module containing the YAML Data and Project IO plugins.""" from __future__ import annotations +from dataclasses import replace from pathlib import Path from typing import TYPE_CHECKING @@ -214,8 +215,11 @@ def save_result( save_model(result.scheme.model, model_path, allow_overwrite=True) paths.append(model_path.as_posix()) + # The source_path attribute of the datasets only gets changed for `result.data` + # Which why we overwrite the data attribute on a copy of `result.scheme` + scheme = replace(result.scheme, data=result.data) scheme_path = result_folder / "scheme.yml" - save_scheme(result.scheme, scheme_path, allow_overwrite=True) + save_scheme(scheme, scheme_path, allow_overwrite=True) paths.append(scheme_path.as_posix()) result_dict = asdict(result, folder=result_folder)