From 0dc72634a928d60c583c0189805cefb1d4f07f7c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Erik=20Fr=C3=B6jdh?= Date: Fri, 18 Sep 2026 17:06:11 +0200 Subject: [PATCH] Reject raw files without padding if discard partial is not set (#374) We have no code in aare to deal with RawFiles that have missing packets if they are not padded to full size or frames with packet loss dropped. Added: - check that we don't try to read files that are not supported - python bindings for frame discard policy at least we get the check in: closes #171 --------- Co-authored-by: AliceMazzoleni99 --- RELEASE.md | 8 ++++- docs/src/RawFile.rst | 8 +++++ docs/src/python/file/pyRawFile.rst | 7 ++++ include/aare/RawFile.hpp | 2 ++ python/aare/__init__.py | 2 +- python/src/bind_Defs.hpp | 5 +++ python/tests/test_RawFile.py | 55 ++++++++++++++++++++++++++++++ python/tests/test_RawMasterFile.py | 17 +++++++-- src/RawFile.cpp | 8 +++++ src/RawFile.test.cpp | 45 ++++++++++++++++++++++++ src/RawMasterFile.cpp | 5 ++- src/RawMasterFile.test.cpp | 27 +++++++++++++++ 12 files changed, 181 insertions(+), 8 deletions(-) diff --git a/RELEASE.md b/RELEASE.md index 25ba39f3..6c0e6ed2 100644 --- a/RELEASE.md +++ b/RELEASE.md @@ -4,6 +4,8 @@ ### New Features: +- Added the Python ``FrameDiscardPolicy`` enum with ``NoDiscard``, ``Discard``, + and ``DiscardPartial``, enabling access to ``RawMasterFile.frame_discard_policy``. - Added the Python ``Pedestal`` factory with ``dtype`` selection, matching ``FastPedestal`` and defaulting to ``float64`` output. - Added ``FastPedestal`` in C++ and Python for per-pixel running mean @@ -74,6 +76,11 @@ - ``TimingMode::Auto`` changed to ``TimingMode::AUTO_TIMING``, ``TimingMode::Trigger`` changed to ``TimingMode::TRIGGER_EXPOSURE`` ### Bugfixes: +- ``RawFile`` and ``File`` reject raw files with frame padding disabled unless + the frame discard policy is ``discardpartial``. The constructor reports the + master path before opening data subfiles. Legacy ``.raw`` master files now + parse the frame discard policy so unpadded ``discardpartial`` files remain + readable. - Fixed ``CtbRawFile.read_frame(index)`` and reads after ``seek(index)`` at subfile boundaries skipping a subfile, returning the wrong frame or raising ``Subfile index out of range``. @@ -265,4 +272,3 @@ dhanya.thattil@psi.ch - diff --git a/docs/src/RawFile.rst b/docs/src/RawFile.rst index bd45ad71..8ecd30e9 100644 --- a/docs/src/RawFile.rst +++ b/docs/src/RawFile.rst @@ -1,6 +1,14 @@ RawFile =============== +Reading requires frame padding to be enabled +or ``Frame Discard Policy`` to be ``discardpartial``. +The constructor checks the +parsed master metadata before opening data subfiles and throws +``std::runtime_error`` with the master path if this requirement is not met. +This applies to JSON and legacy ``.raw`` masters, including when opened through +``aare::File``. ``RawMasterFile`` can still be used to inspect their metadata. + ``total_frames()`` is the minimum actual frame count across all selected ``RawSubFile`` objects in all ROIs. Each subfile count includes its complete series of ``.raw`` files and is determined when the file is opened. Disabled diff --git a/docs/src/python/file/pyRawFile.rst b/docs/src/python/file/pyRawFile.rst index deeb8f3e..a128d2fe 100644 --- a/docs/src/python/file/pyRawFile.rst +++ b/docs/src/python/file/pyRawFile.rst @@ -3,6 +3,13 @@ RawFile .. py:currentmodule:: aare +Reading requires frame padding to be enabled +or ``Frame Discard Policy`` to be ``discardpartial``. Construction raises +``RuntimeError`` with the master path before opening data subfiles if this +requirement is not met. This applies to JSON and legacy ``.raw`` masters, +including when opened through ``File``. ``RawMasterFile`` can still be used +to inspect their metadata. + ``total_frames`` and ``len(reader)`` report the minimum actual frame count across all selected subfiles and ROIs, including each subfile's complete ``.raw`` series. ``read()`` uses this count, and ``read_n()`` and diff --git a/include/aare/RawFile.hpp b/include/aare/RawFile.hpp index 3b27e156..4ef92962 100644 --- a/include/aare/RawFile.hpp +++ b/include/aare/RawFile.hpp @@ -39,6 +39,8 @@ class RawFile : public FileInterface { * @brief RawFile constructor * @param fname path to the master file (.json) * @param mode file mode (only "r" is supported at the moment) + * @throws std::runtime_error if frame padding is disabled and the frame + * discard policy is not DiscardPartial. */ RawFile(const std::filesystem::path &fname, const std::string &mode = "r"); virtual ~RawFile() override = default; diff --git a/python/aare/__init__.py b/python/aare/__init__.py index ecd111c9..b4be0755 100644 --- a/python/aare/__init__.py +++ b/python/aare/__init__.py @@ -22,7 +22,7 @@ from ._aare import ( RawSubFile, ) from ._aare import Pedestal_d, Pedestal_f, ClusterFinder_Cluster3x3i, VarClusterFinder -from ._aare import DetectorType, ReadoutMode +from ._aare import DetectorType, FrameDiscardPolicy, ReadoutMode from ._aare import hitmap from ._aare import ROI from ._aare import corner diff --git a/python/src/bind_Defs.hpp b/python/src/bind_Defs.hpp index 2b25669b..c6e7d185 100644 --- a/python/src/bind_Defs.hpp +++ b/python/src/bind_Defs.hpp @@ -95,4 +95,9 @@ void define_defs_bindings(py::module &m) { .value("GATED", TimingMode::GATED) .value("BURST_TRIGGER", TimingMode::BURST_TRIGGER) .value("TRIGGER_GATED", TimingMode::TRIGGER_GATED); + + py::enum_(m, "FrameDiscardPolicy") + .value("NoDiscard", FrameDiscardPolicy::NoDiscard) + .value("Discard", FrameDiscardPolicy::Discard) + .value("DiscardPartial", FrameDiscardPolicy::DiscardPartial); } diff --git a/python/tests/test_RawFile.py b/python/tests/test_RawFile.py index 78636a9c..1fb6c3f7 100644 --- a/python/tests/test_RawFile.py +++ b/python/tests/test_RawFile.py @@ -28,6 +28,61 @@ def small_raw_file(tmp_path): return master_path +@pytest.mark.parametrize("reader_type", [RawFile, File]) +@pytest.mark.parametrize("legacy_master", [False, True]) +@pytest.mark.parametrize("padding, policy, supported", [ + (0, "nodiscard", False), + (0, "discard", False), + (0, "discardpartial", True), + (1, "nodiscard", True), + (1, "discard", True), + (1, "discardpartial", True), +]) +def test_raw_frame_policy(small_raw_file, reader_type, legacy_master, + padding, policy, supported): + master_path = small_raw_file + if legacy_master: + master_path = master_path.with_suffix(".raw") + master_path.write_text( + "Version : 6.4\n" + "Detector Type : Jungfrau\n" + "Timing Mode : auto\n" + "Geometry : [1, 1]\n" + "Image Size : 12\n" + "Pixels : [3, 2]\n" + "Dynamic Range : 16\n" + "Max Frames Per File : 1\n" + "Total Frames : 2\n" + "Frames in File : 2\n" + f"Frame Padding : {padding}\n" + f"Frame Discard Policy : {policy}\n" + ) + else: + metadata = json.loads(master_path.read_text()) + metadata["Frame Padding"] = padding + metadata["Frame Discard Policy"] = policy + master_path.write_text(json.dumps(metadata)) + + if supported: + reader = reader_type(master_path) + assert reader.total_frames == 2 + frame = reader.read_frame() + if reader_type is RawFile: + _, frame = frame + np.testing.assert_array_equal(frame, np.arange(6).reshape(2, 3)) + else: + message = "requires frame padding or discardpartial" + with pytest.raises(RuntimeError, match=message) as error: + reader_type(master_path) + assert str(master_path) in str(error.value) + + for index in range(2): + (master_path.parent / f"run_d0_f{index}_0.raw").unlink() + with pytest.raises(RuntimeError, match=message) as error: + reader_type(master_path) + assert str(master_path) in str(error.value) + + @pytest.mark.parametrize("method, args, kwargs", [ ("read_frame", (), {}), ("read_n", (1,), {}), diff --git a/python/tests/test_RawMasterFile.py b/python/tests/test_RawMasterFile.py index 8ca8aee1..5ad68b48 100644 --- a/python/tests/test_RawMasterFile.py +++ b/python/tests/test_RawMasterFile.py @@ -3,7 +3,7 @@ import json import pytest -from aare import RawMasterFile, ReadoutMode, DetectorType +from aare import RawMasterFile, ReadoutMode, DetectorType, FrameDiscardPolicy @pytest.mark.withdata @@ -16,7 +16,15 @@ def test_read_rawfile_quad_eiger_and_compare_to_numpy(test_data_path): assert(f.detector_type == DetectorType.Jungfrau) -def test_raw_master_file_context_manager(tmp_path): +@pytest.mark.parametrize( + "policy, expected_policy", + [ + ("nodiscard", FrameDiscardPolicy.NoDiscard), + ("discard", FrameDiscardPolicy.Discard), + ("discardpartial", FrameDiscardPolicy.DiscardPartial), + ], +) +def test_raw_master_file_context_manager(tmp_path, policy, expected_policy): file_name = tmp_path / "run_master_0.json" file_name.write_text(json.dumps({ "Version": 7.2, @@ -29,10 +37,13 @@ def test_raw_master_file_context_manager(tmp_path): "Total Frames": 2, "Frames in File": 2, "Frame Padding": 1, - "Frame Discard Policy": "nodiscard", + "Frame Discard Policy": policy, })) with RawMasterFile(file_name) as context_file: assert context_file.reading_mode == ReadoutMode.UNKNOWN assert context_file.detector_type == DetectorType.Jungfrau + frame_discard_policy = context_file.frame_discard_policy + assert isinstance(frame_discard_policy, FrameDiscardPolicy) + assert frame_discard_policy == expected_policy diff --git a/src/RawFile.cpp b/src/RawFile.cpp index 1d76b622..1e33bf83 100644 --- a/src/RawFile.cpp +++ b/src/RawFile.cpp @@ -29,6 +29,14 @@ RawFile::RawFile(const std::filesystem::path &fname, const std::string &mode) m_mode = mode; if (mode == "r") { + if (m_master.frame_padding() == 0 && + m_master.frame_discard_policy() != + FrameDiscardPolicy::DiscardPartial) { + throw std::runtime_error(fmt::format( + "Cannot open '{}': RawFile requires frame padding or " + "discardpartial.", + m_master.master_fname().string())); + } m_subfiles.resize(m_master.roi_geometries().size()); // iterate over all ROIS diff --git a/src/RawFile.test.cpp b/src/RawFile.test.cpp index e798a610..ee0b9e24 100644 --- a/src/RawFile.test.cpp +++ b/src/RawFile.test.cpp @@ -16,6 +16,51 @@ using aare::File; using aare::RawFile; using namespace aare; +TEST_CASE("RawFile requires padding or discarding partial frames", + "[RawFile][frame-policy]") { + const auto [padding, policy, supported] = + GENERATE(table({ + {0, "nodiscard", false}, + {0, "discard", false}, + {0, "discardpartial", true}, + {1, "nodiscard", true}, + {1, "discard", true}, + {1, "discardpartial", true}, + })); + CAPTURE(padding, policy); + TemporaryRawFiles files; + nlohmann::json metadata; + std::ifstream(files.master_path()) >> metadata; + metadata["Frame Padding"] = padding; + metadata["Frame Discard Policy"] = policy; + std::ofstream(files.master_path()) << metadata; + + REQUIRE(RawMasterFile(files.master_path()).frame_padding() == padding); + if (supported) { + RawFile reader(files.master_path()); + REQUIRE(reader.total_frames() == 2); + auto frame = reader.read_frame(); + REQUIRE(frame.view()(0, 0) == 1); + File generic_reader(files.master_path()); + auto generic_frame = generic_reader.read_frame(); + REQUIRE(generic_frame.view()(0, 0) == 1); + } else { + SECTION("with data subfiles") {} + SECTION("without data subfiles") { + std::filesystem::remove(files.data_path(0, 0)); + std::filesystem::remove(files.data_path(0, 1)); + } + const auto path_matcher = + Catch::Matchers::ContainsSubstring(files.master_path().string()); + const auto policy_matcher = Catch::Matchers::ContainsSubstring( + "requires frame padding or discardpartial"); + const auto message = path_matcher && policy_matcher; + REQUIRE_THROWS_AS(RawFile(files.master_path()), std::runtime_error); + REQUIRE_THROWS_WITH(RawFile(files.master_path()), message); + REQUIRE_THROWS_WITH(File(files.master_path()), message); + } +} + TEST_CASE("RawFile read errors identify the frame index and master path", "[RawFile][read-errors]") { TemporaryRawFiles files; diff --git a/src/RawMasterFile.cpp b/src/RawMasterFile.cpp index 61999438..e0880ba5 100644 --- a/src/RawMasterFile.cpp +++ b/src/RawMasterFile.cpp @@ -546,9 +546,8 @@ void RawMasterFile::parse_raw(std::istream &is) { m_image_size_in_bytes = std::stoi(value); } else if (key == "Frame Padding") { m_frame_padding = std::stoi(value); - // } else if (key == "Frame Discard Policy"){ - // m_frame_discard_policy = - // StringTo(value); + } else if (key == "Frame Discard Policy") { + m_frame_discard_policy = string_to(value); // } else if (key == "Number of rows"){ // m_number_of_rows = std::stoi(value); } else if (key == "Analog Flag") { diff --git a/src/RawMasterFile.test.cpp b/src/RawMasterFile.test.cpp index bd244844..3838bfc2 100644 --- a/src/RawMasterFile.test.cpp +++ b/src/RawMasterFile.test.cpp @@ -3,11 +3,38 @@ #include "test_config.hpp" #include +#include #include #include using namespace aare; +TEST_CASE("Parse legacy master frame discard policies", + "[rawmasterfile][frame-policy]") { + const auto [policy, expected] = + GENERATE(table({ + {"nodiscard", FrameDiscardPolicy::NoDiscard}, + {"discard", FrameDiscardPolicy::Discard}, + {"discardpartial", FrameDiscardPolicy::DiscardPartial}, + {"", FrameDiscardPolicy::NoDiscard}, + })); + CAPTURE(policy); + std::ostringstream content; + content << "Version : 6.4\n" + "Detector Type : Jungfrau\n" + "Timing Mode : auto\n" + "Geometry : [1, 1]\n" + "Pixels : [3, 2]\n" + "Frame Padding : 0\n"; + if (!policy.empty()) { + content << "Frame Discard Policy : " << policy << '\n'; + } + std::istringstream input(content.str()); + RawMasterFile master(input, "run_master_0.raw"); + REQUIRE(master.frame_padding() == 0); + REQUIRE(master.frame_discard_policy() == expected); +} + TEST_CASE("Parse a master file fname") { RawFileNameComponents m("test_master_1.json"); REQUIRE(m.base_name() == "test");