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) diff --git a/glotaran/builtin/io/yml/test/test_save_result.py b/glotaran/builtin/io/yml/test/test_save_result.py index 45dec2616..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,26 +10,32 @@ 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 from glotaran.testing.simulated_data.sequential_spectral_decay import SCHEME +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) -def test_save_result_yml( - tmp_path: Path, - dummy_result: Result, -): + yield create_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 +56,86 @@ 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") + + assert ( + dummy_result.scheme.data["dataset_1"].source_path == (tmp_path / "data/ds1.nc").as_posix() + ) - 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() + # 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() - 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) 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) diff --git a/glotaran/utils/io.py b/glotaran/utils/io.py index 765e8e3cd..83e674cf6 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,9 +192,42 @@ 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. + 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. @@ -201,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 d03d7ab1b..d0c5c103f 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,51 @@ 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}" + + 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.""" + 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")