From 1821659405afc74183210ab984f35c08861cd21b Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Sat, 15 Aug 2026 00:32:10 +0800 Subject: [PATCH 1/4] BUG: refuse to run a Monte Carlo over a results file it cannot write import_outputs() accepts .csv and .json, points output_file at the file, and offers continuing a simulation from it. simulate() only writes JSONL, and __setup_files opens with w+ when append is False, so the imported file was truncated and then filled with records its own extension does not describe. Checked before any file is opened, and named per path so the message says which one has to change. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- rocketpy/simulation/monte_carlo.py | 32 ++++++++++++++ tests/unit/simulation/test_monte_carlo.py | 52 +++++++++++++++++++++++ 2 files changed, 84 insertions(+) diff --git a/rocketpy/simulation/monte_carlo.py b/rocketpy/simulation/monte_carlo.py index 21c665d01..b6d55bd28 100644 --- a/rocketpy/simulation/monte_carlo.py +++ b/rocketpy/simulation/monte_carlo.py @@ -38,6 +38,33 @@ # TODO: Create evolution plots to analyze convergence +# simulate() writes one JSON object per line and reads that same shape back, so +# this is the only format it can both resume from and overwrite safely. +_SIMULATION_LOG_SUFFIX = ".txt" + + +def _refuse_logs_this_run_cannot_write(input_file, output_file, error_file): + """Reject a log file ``simulate`` would damage rather than extend. + + A ``.csv`` or ``.json`` is importable for analysis, but this run would + truncate it under ``append=False`` and leave it half one format and half + another under ``append=True``. Checked before any file is opened. + """ + for label, path in ( + ("input_file", input_file), + ("output_file", output_file), + ("error_file", error_file), + ): + if Path(path).suffix.lower() != _SIMULATION_LOG_SUFFIX: + raise ValueError( + f"Monte Carlo simulation logs must be {_SIMULATION_LOG_SUFFIX} " + f"files holding one JSON object per line; {label} is " + f"'{path}'. CSV and JSON results can be imported for analysis, " + f"but simulate() cannot resume from or overwrite them. Point " + f"{label} at a {_SIMULATION_LOG_SUFFIX} file to run." + ) + + class MonteCarlo: # pylint: disable=too-many-public-methods """Class to run a Monte Carlo simulation of a rocket flight. @@ -223,6 +250,11 @@ def simulate( self.number_of_simulations = number_of_simulations self._initial_sim_idx = self.num_of_loaded_sims if append else 0 + # Before anything is opened: __setup_files truncates for append=False. + _refuse_logs_this_run_cannot_write( + self.input_file, self.output_file, self.error_file + ) + print("Starting Monte Carlo analysis") self.__setup_files(append) diff --git a/tests/unit/simulation/test_monte_carlo.py b/tests/unit/simulation/test_monte_carlo.py index 7e2e68804..6d758e4a7 100644 --- a/tests/unit/simulation/test_monte_carlo.py +++ b/tests/unit/simulation/test_monte_carlo.py @@ -8,6 +8,9 @@ import pytest from rocketpy.simulation import MonteCarlo +from rocketpy.simulation.monte_carlo import ( + _refuse_logs_this_run_cannot_write, +) plt.rcParams.update({"figure.max_open_warning": 0}) @@ -513,3 +516,52 @@ def test_simulate_convergence_runs_until_max_when_not_converging(): assert mc.num_of_loaded_sims == 200 assert all(width > 0.5 for width in history) assert len(history) == 4 # 200 / 50 batches + + +@pytest.mark.parametrize( + "suffix, payload", + [ + (".csv", "apogee,index\n1234.0,0\n1250.0,1\n"), + (".json", '[{"apogee": 1234.0, "index": 0}]\n'), + ], +) +@pytest.mark.parametrize("append", [False, True]) +def test_simulate_refuses_a_results_file_it_cannot_write( + monte_carlo_calisto, tmp_path, suffix, payload, append +): + """Importing CSV or JSON results must not let simulate() write over them. + + ``import_outputs`` accepts both and points ``output_file`` at the file, and + its docstring offers continuing a simulation. simulate() only writes JSONL, + so ``append=False`` truncated the file before this check existed. + """ + results = tmp_path / f"results{suffix}" + results.write_text(payload, encoding="utf-8") + monte_carlo_calisto.output_file = str(results) + before = results.read_bytes() + + with pytest.raises(ValueError, match="one JSON object per line"): + monte_carlo_calisto.simulate(number_of_simulations=1, append=append) + + assert results.read_bytes() == before + + +def test_simulation_log_check_names_the_file_that_is_wrong(tmp_path): + """The message says which of the three paths has to change.""" + good = str(tmp_path / "run.txt") + + _refuse_logs_this_run_cannot_write(good, good, good) # canonical, no raise + + for label, args in ( + ("input_file", (str(tmp_path / "a.csv"), good, good)), + ("output_file", (good, str(tmp_path / "b.json"), good)), + ("error_file", (good, good, str(tmp_path / "c.csv"))), + ): + with pytest.raises(ValueError, match=label): + _refuse_logs_this_run_cannot_write(*args) + + +def test_simulation_log_check_accepts_an_uppercase_suffix(tmp_path): + """A .TXT log is the same file to the filesystem, so it is accepted.""" + upper = str(tmp_path / "run.TXT") + _refuse_logs_this_run_cannot_write(upper, upper, upper) From fc2a09346e8e38de5ec9514030ddbfc2ce460f64 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Sat, 15 Aug 2026 01:43:38 +0800 Subject: [PATCH 2/4] DOC: stop offering CSV and JSON results as something to resume from The note under import_outputs said any previously saved file could be used to continue a simulation, which is what led a .csv into output_file in the first place. Say which format that holds for. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- monte_carlo_test.errors.txt | 0 monte_carlo_test.inputs.txt | 0 monte_carlo_test.outputs.txt | 0 rocketpy/simulation/monte_carlo.py | 4 +++- 4 files changed, 3 insertions(+), 1 deletion(-) create mode 100644 monte_carlo_test.errors.txt create mode 100644 monte_carlo_test.inputs.txt create mode 100644 monte_carlo_test.outputs.txt diff --git a/monte_carlo_test.errors.txt b/monte_carlo_test.errors.txt new file mode 100644 index 000000000..e69de29bb diff --git a/monte_carlo_test.inputs.txt b/monte_carlo_test.inputs.txt new file mode 100644 index 000000000..e69de29bb diff --git a/monte_carlo_test.outputs.txt b/monte_carlo_test.outputs.txt new file mode 100644 index 000000000..e69de29bb diff --git a/rocketpy/simulation/monte_carlo.py b/rocketpy/simulation/monte_carlo.py index b6d55bd28..5bb926000 100644 --- a/rocketpy/simulation/monte_carlo.py +++ b/rocketpy/simulation/monte_carlo.py @@ -1248,7 +1248,9 @@ def import_outputs(self, filename=None): ----- Notice that you can import the outputs, inputs, and errors from a file without the need to run simulations. You can use previously saved - files to process analyze the results or to continue a simulation. + files to process and analyze the results, and a ``.txt`` one to continue + a simulation. A ``.csv`` or ``.json`` is read-only here: ``simulate`` + writes JSONL and refuses to run over a file it could not read back. """ filepath = filename if filename else self.filename.with_suffix(".outputs.txt") From b89ca0e2b25d9d4ce3e97c94d421dfa3dfe919a6 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Sat, 15 Aug 2026 04:49:28 +0800 Subject: [PATCH 3/4] BUG: refuse working logs that are one file, and options that split a row import_results points input_file, output_file and error_file at one path, and a run then appends input rows and output rows into it. Compared by inode once the files exist, so a symlink, a hard link, a/../run.txt and a case-insensitive filesystem are all the same file rather than three names. json.dumps kwargs reach the writer, so indent=2 wrote records across several lines while every reader here takes one line at a time. The run finished and the completeness check then called the file it had just written damaged. indent of 0 and "" do the same, as does a newline inside separators. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- rocketpy/simulation/monte_carlo.py | 71 ++++++++++++++++- tests/unit/simulation/test_monte_carlo.py | 96 +++++++++++++++++++++-- 2 files changed, 158 insertions(+), 9 deletions(-) diff --git a/rocketpy/simulation/monte_carlo.py b/rocketpy/simulation/monte_carlo.py index 5bb926000..d64a1fa2b 100644 --- a/rocketpy/simulation/monte_carlo.py +++ b/rocketpy/simulation/monte_carlo.py @@ -43,7 +43,9 @@ _SIMULATION_LOG_SUFFIX = ".txt" -def _refuse_logs_this_run_cannot_write(input_file, output_file, error_file): +def _refuse_logs_this_run_cannot_write( + input_file, output_file, error_file, export_config=None +): """Reject a log file ``simulate`` would damage rather than extend. A ``.csv`` or ``.json`` is importable for analysis, but this run would @@ -64,6 +66,71 @@ def _refuse_logs_this_run_cannot_write(input_file, output_file, error_file): f"{label} at a {_SIMULATION_LOG_SUFFIX} file to run." ) + _refuse_logs_that_are_one_file( + ( + ("input_file", input_file), + ("output_file", output_file), + ("error_file", error_file), + ) + ) + _refuse_export_options_that_break_a_line(export_config or {}) + + +def _points_at_the_same_file(one, other): + """Whether two names reach one file, by inode when both already exist. + + ``samefile`` settles symlinks, hard links and a case-insensitive filesystem, + none of which text comparison sees. It needs both to exist, so a run that has + not created them yet falls back to the resolved paths, which still normalises + ``a/../run.txt`` and any symlinked parent. + """ + one, other = Path(one), Path(other) + try: + return one.samefile(other) + except OSError: + return one.resolve() == other.resolve() + + +def _refuse_logs_that_are_one_file(labelled_paths): + """Each log has to be its own file, however the three were named. + + ``import_results`` points all three at one path, and the run then appends + input rows and output rows into it. The completeness check reports the mess + afterwards, by which time the file it was given is already gone. + """ + for index, (label, path) in enumerate(labelled_paths): + for other_label, other in labelled_paths[index + 1 :]: + if _points_at_the_same_file(path, other): + raise ValueError( + f"{label} and {other_label} are the same file ('{path}' and " + f"'{other}'). A run appends input rows and output rows " + f"separately, so sharing one log writes both into it and " + f"leaves neither readable. Give each its own file." + ) + + +def _refuse_export_options_that_break_a_line(export_config): + """Reject export options that would split one record over several lines. + + The logs hold one JSON object per line and every reader here assumes it, so + ``indent`` of any kind, ``0`` and ``""`` included, leaves a file that the + completeness check calls damaged once the run it just finished is over. + """ + if export_config.get("indent") is not None: + raise ValueError( + f"indent={export_config['indent']!r} cannot be used with a Monte " + f"Carlo run: the logs hold one JSON object per line, and an " + f"indented record spans several. Export the results with indent " + f"after the run instead." + ) + separators = export_config.get("separators") + if separators and any("\n" in str(part) for part in separators): + raise ValueError( + f"separators={separators!r} cannot be used with a Monte Carlo run: " + f"a newline inside a record splits it across lines, and the logs " + f"hold one JSON object per line." + ) + class MonteCarlo: # pylint: disable=too-many-public-methods """Class to run a Monte Carlo simulation of a rocket flight. @@ -252,7 +319,7 @@ def simulate( # Before anything is opened: __setup_files truncates for append=False. _refuse_logs_this_run_cannot_write( - self.input_file, self.output_file, self.error_file + self.input_file, self.output_file, self.error_file, kwargs ) print("Starting Monte Carlo analysis") diff --git a/tests/unit/simulation/test_monte_carlo.py b/tests/unit/simulation/test_monte_carlo.py index 6d758e4a7..72c7845db 100644 --- a/tests/unit/simulation/test_monte_carlo.py +++ b/tests/unit/simulation/test_monte_carlo.py @@ -1,5 +1,6 @@ import csv import json +import os import pathlib from collections import namedtuple @@ -546,16 +547,23 @@ def test_simulate_refuses_a_results_file_it_cannot_write( assert results.read_bytes() == before +def _three_logs(tmp_path): + """Three distinct, acceptable working logs.""" + return [ + str(tmp_path / f"run.{part}.txt") for part in ("inputs", "outputs", "errors") + ] + + def test_simulation_log_check_names_the_file_that_is_wrong(tmp_path): """The message says which of the three paths has to change.""" - good = str(tmp_path / "run.txt") + good = _three_logs(tmp_path) - _refuse_logs_this_run_cannot_write(good, good, good) # canonical, no raise + _refuse_logs_this_run_cannot_write(*good) # canonical, no raise for label, args in ( - ("input_file", (str(tmp_path / "a.csv"), good, good)), - ("output_file", (good, str(tmp_path / "b.json"), good)), - ("error_file", (good, good, str(tmp_path / "c.csv"))), + ("input_file", (str(tmp_path / "a.csv"), good[1], good[2])), + ("output_file", (good[0], str(tmp_path / "b.json"), good[2])), + ("error_file", (good[0], good[1], str(tmp_path / "c.csv"))), ): with pytest.raises(ValueError, match=label): _refuse_logs_this_run_cannot_write(*args) @@ -563,5 +571,79 @@ def test_simulation_log_check_names_the_file_that_is_wrong(tmp_path): def test_simulation_log_check_accepts_an_uppercase_suffix(tmp_path): """A .TXT log is the same file to the filesystem, so it is accepted.""" - upper = str(tmp_path / "run.TXT") - _refuse_logs_this_run_cannot_write(upper, upper, upper) + upper = [str(tmp_path / f"run.{part}.TXT") for part in ("in", "out", "err")] + _refuse_logs_this_run_cannot_write(*upper) + + +def _three_logs(tmp_path): + """Three distinct, acceptable working logs.""" + return [ + str(tmp_path / f"run.{part}.txt") for part in ("inputs", "outputs", "errors") + ] + + +def test_working_logs_must_be_three_different_files(tmp_path): + """``import_results`` points all three at one path, which cannot work. + + A run appends input rows and output rows separately, so one shared log ends + up holding both and neither reader can make sense of it. + """ + shared = str(tmp_path / "result.txt") + + with pytest.raises(ValueError, match="same file"): + _refuse_logs_this_run_cannot_write(shared, shared, shared) + + +@pytest.mark.parametrize("alias", ["dotdot", "symlink", "hardlink"]) +def test_a_log_named_two_ways_is_still_one_file(tmp_path, alias): + """Text comparison misses every way one file answers to two names.""" + inputs, _, errors = _three_logs(tmp_path) + pathlib.Path(inputs).write_text("", encoding="utf-8") + (tmp_path / "sub").mkdir() + + if alias == "dotdot": + other = str(tmp_path / "sub" / ".." / "run.inputs.txt") + else: + other = str(tmp_path / f"run.{alias}.txt") + try: + if alias == "symlink": + pathlib.Path(other).symlink_to(inputs) + else: + os.link(inputs, other) + except (OSError, NotImplementedError): + pytest.skip(f"{alias} not available on this filesystem") + + with pytest.raises(ValueError, match="same file"): + _refuse_logs_this_run_cannot_write(inputs, other, errors) + + +def test_three_separate_logs_are_accepted(tmp_path): + """The control: distinct .txt paths raise nothing.""" + _refuse_logs_this_run_cannot_write(*_three_logs(tmp_path)) + + +@pytest.mark.parametrize("indent", [2, 0, ""]) +def test_an_indented_record_is_refused_before_anything_is_written(tmp_path, indent): + """``indent`` splits a record over lines the readers take one at a time. + + Without this the run finished, then the completeness check called the file + it had just written damaged. + """ + with pytest.raises(ValueError, match="indent"): + _refuse_logs_this_run_cannot_write(*_three_logs(tmp_path), {"indent": indent}) + + +def test_a_newline_in_the_separators_is_refused_too(tmp_path): + """The same hazard by another name.""" + with pytest.raises(ValueError, match="separators"): + _refuse_logs_this_run_cannot_write( + *_three_logs(tmp_path), {"separators": (",\n", ": ")} + ) + + +@pytest.mark.parametrize( + "harmless", [{"indent": None}, {"sort_keys": True}, {"ensure_ascii": False}] +) +def test_export_options_that_keep_one_line_are_left_alone(tmp_path, harmless): + """Only what puts a newline inside a record is refused.""" + _refuse_logs_this_run_cannot_write(*_three_logs(tmp_path), harmless) From 1c9ab8c03c17f0fe597e414a41394408eb57165e Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Sat, 15 Aug 2026 06:02:58 +0800 Subject: [PATCH 4/4] TST: cover the branch a first run actually takes samefile needs both files to exist, and every case here wrote one first, so the resolved-path fallback that a run with no logs yet goes through was never exercised. Replacing it with False leaves the new test red. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- monte_carlo_test.errors.txt | 0 monte_carlo_test.inputs.txt | 0 monte_carlo_test.outputs.txt | 0 tests/unit/simulation/test_monte_carlo.py | 16 ++++++++++++++++ 4 files changed, 16 insertions(+) delete mode 100644 monte_carlo_test.errors.txt delete mode 100644 monte_carlo_test.inputs.txt delete mode 100644 monte_carlo_test.outputs.txt diff --git a/monte_carlo_test.errors.txt b/monte_carlo_test.errors.txt deleted file mode 100644 index e69de29bb..000000000 diff --git a/monte_carlo_test.inputs.txt b/monte_carlo_test.inputs.txt deleted file mode 100644 index e69de29bb..000000000 diff --git a/monte_carlo_test.outputs.txt b/monte_carlo_test.outputs.txt deleted file mode 100644 index e69de29bb..000000000 diff --git a/tests/unit/simulation/test_monte_carlo.py b/tests/unit/simulation/test_monte_carlo.py index 72c7845db..bd27db221 100644 --- a/tests/unit/simulation/test_monte_carlo.py +++ b/tests/unit/simulation/test_monte_carlo.py @@ -647,3 +647,19 @@ def test_a_newline_in_the_separators_is_refused_too(tmp_path): def test_export_options_that_keep_one_line_are_left_alone(tmp_path, harmless): """Only what puts a newline inside a record is refused.""" _refuse_logs_this_run_cannot_write(*_three_logs(tmp_path), harmless) + + +def test_two_names_for_a_file_that_does_not_exist_yet_are_still_one_file(tmp_path): + """``samefile`` needs both to exist, and a first run has created neither. + + Every other case here writes the file first, so the resolved-path branch + that a first run actually takes was never exercised. + """ + (tmp_path / "sub").mkdir() + missing = str(tmp_path / "run.inputs.txt") + same_by_another_name = str(tmp_path / "sub" / ".." / "run.inputs.txt") + errors = str(tmp_path / "run.errors.txt") + + assert not pathlib.Path(missing).exists() + with pytest.raises(ValueError, match="same file"): + _refuse_logs_this_run_cannot_write(missing, same_by_another_name, errors)