Refactoring and simplifications:

- Adjust for reduced remap result struct
- Add predefined SensorConfigs
- Move combine_group_maps to implementation detail of RemapGenerate
- Remove apply_rotation_shift and Rotation flip (have become superfluous)

include/aare/RemapAlgorithm.hpp

@@ -4,23 +4,6 @@
 
 namespace aare::remap::algo {
 
-constexpr defs::Rotation flip(defs::Rotation r) noexcept {
-    switch (r) {
-    case defs::Rotation::Identity:
-        return defs::Rotation::Rotate180;
-    case defs::Rotation::Rotate180:
-        return defs::Rotation::Identity;
-    }
-
-    assert(false && "Invalid Rotation passed to flip");
-    return defs::Rotation::Identity; // Unreachable; satisfies compiler
-}
-
-// Is it better to pass defs::GroupConfig const& and return a copy?
-void apply_rotation_shift(defs::GroupConfig &,
-                          defs::SensorPixelGeometry const &, defs::BondShift,
-                          defs::Rotation);
-
 defs::StrixelGroupToPixelMap strixel_to_pixel_map(
     defs::GroupConfig const &, defs::SensorPixelGeometry const &,
     defs::SensorModulePlacement const &, InclusiveROI const &user_roi,
@@ -31,10 +14,6 @@ strixel_to_pixel_maps(defs::SensorConfig const &, defs::SensorModulePlacement co
                       InclusiveROI const &user_roi,
                       defs::BondShift bond_shift = {0, 0});
 
-defs::StrixelGroupToPixelMap
-combine_maps(std::vector<defs::StrixelGroupToPixelMap> const &,
-             std::vector<int> const &);
-
 /**
  *  Public API:
  *  Applies a given remapping rule to an input array.

include/aare/RemapConfig.hpp

@@ -68,8 +68,15 @@ inline constexpr defs::GroupConfig SingleChipMP_iLGAD_P18{
  * Number of strixel rows: 476
  ********************************/
 
+// Complete SensorConfig with all groups
+inline const defs::SensorConfig SingleChipMP_iLGAD{
+    // NOTE: Vector cannot be constexpr
+    .pixel = SingleChipMP_iLGAD_pix,
+    .group_configs = {SingleChipMP_iLGAD_P25, SingleChipMP_iLGAD_P15,
+                      SingleChipMP_iLGAD_P18}};
+
 /************************************
- * Single chip, multi-pitch, TEW
+ * Single chip, multi-pitch, Thin Entrance Window (TEW)
  ************************************/
 inline constexpr defs::SensorPixelGeometry SingleChipMP_TEW_pix{
     .num_pix_x = 256, .num_pix_y = 256, .guardring = {.x = 0, .y = 0}};
@@ -109,12 +116,21 @@ inline constexpr defs::GroupConfig SingleChipMP_TEW_P18{
  * Number of strixel rows: 512
  ********************************/
 
+// Complete SensorConfig with all groups
+inline const defs::SensorConfig SingleChipMP_TEW{
+    // NOTE: Vector cannot be constexpr
+    .pixel = SingleChipMP_TEW_pix,
+    .group_configs = {SingleChipMP_TEW_P25, SingleChipMP_TEW_P15,
+                      SingleChipMP_TEW_P18}};
+
 /************************************
  * Quad, 25 um, iLGAD
  ************************************/
 inline constexpr defs::SensorPixelGeometry Quad_iLGAD_pix{
     .num_pix_x = 512, .num_pix_y = 512, .guardring = {.x = 9, .y = 9}};
 
+inline constexpr int Quad_iLGAD_strixel_gap_rows = 12;
+
 inline constexpr defs::GroupConfig Quad_iLGAD_bottomhalf{
     .strixel = StrxP25,
     .placement_on_sensor = {Quad_iLGAD_pix.guardring.x + 2, // 11
@@ -126,10 +142,15 @@ inline constexpr defs::GroupConfig Quad_iLGAD_bottomhalf{
 inline constexpr defs::GroupConfig Quad_iLGAD_tophalf{
     .strixel = StrxP25,
     .routing = {defs::ModuloOrdering::Reverse},
-    // Adapt placement to be correct!
     .placement_on_sensor = {
         Quad_iLGAD_pix.guardring.x + 2,                              // 11
         Quad_iLGAD_pix.num_pix_x - Quad_iLGAD_pix.guardring.x - 1,   // 502
         Quad_iLGAD_pix.num_pix_y / 2 + 1,                            // 257
         Quad_iLGAD_pix.num_pix_y - Quad_iLGAD_pix.guardring.y - 1}}; // 502
+
+// Complete SensorConfig with all groups
+inline const defs::SensorConfig Quad_iLGAD{
+    // NOTE: Vector cannot be constexpr
+    .pixel = Quad_iLGAD_pix,
+    .group_configs = {Quad_iLGAD_bottomhalf, Quad_iLGAD_tophalf}};
 } // namespace aare::remap::config::jungfrau

include/aare/RemapGenerate.hpp

@@ -5,33 +5,134 @@
 
 namespace aare::remap::generate {
 
+// ============================================================
+// Internal implementation details
+// ============================================================
+// Current quick-and-dirty inclusion of a helper function to combine group maps.
+// (Used to live as `combine_maps` in RemapAlgorithm, but arguably should not be
+// part of that public API). Eventually, one could instead split RemapGenerate
+// into hpp and cpp and make this one live in an unnamed namespace inside the
+// cpp so that it cannot be exposed to the user through the public API (if we
+// want to avoid that)
+namespace detail {
+/**
+ * @brief Combine TWO vertically ordered group remapping maps.
+ *
+ * Concatenates the maps in the order provided, inserting `gap_rows`
+ * rows of invalid entries (-1) between consecutive groups.
+ *
+ * The input maps must be ordered in the desired output order.
+ * Both maps must have the same number of columns.
+ *
+ * The returned effective ROI is the bounding ROI covering the
+ * effective ROIs of both input groups. It describes the physical
+ * source-pixel region and does not encode the artificial strixel
+ * gap rows in the output map.
+ *
+ * @param first Group map that comes first in output space.
+ * @param second Group map that comes second in output space.
+ * @param gap_rows Number of invalid strixel rows inserted between groups.
+ *
+ * @return Combined strixel-to-pixel map.
+ *
+ * @throws std::invalid_argument if gap_rows is negative.
+ * @throws std::logic_error if group maps have different widths.
+ */
+defs::StrixelGroupToPixelMap
+combine_group_maps(defs::StrixelGroupToPixelMap const &first,
+                   defs::StrixelGroupToPixelMap const &second, int gap_rows) {
+
+    if (gap_rows < 0) {
+        throw std::invalid_argument("gap_rows must be non-negative");
+    }
+
+    const ssize_t ncols = first.map.shape(1);
+
+    // Make sure both maps have the same width.
+    if (second.map.shape(1) != ncols) {
+        throw std::logic_error("Cannot combine maps with different numbers "
+                               "of columns");
+    }
+
+    // Check effective ROIs line up
+    if (first.effective_roi.xmin != second.effective_roi.xmin ||
+        first.effective_roi.xmax != second.effective_roi.xmax) {
+        throw std::logic_error(
+            "Cannot combine group maps with different x extents");
+    }
+
+    // Calculate total number of output rows.
+    ssize_t total_rows = 0;
+    total_rows = first.map.shape(0) + second.map.shape(0) +
+                 static_cast<ssize_t>(gap_rows);
+
+    // Allocate and initialize with -1.
+    //
+    // -1 represents an output strixel position that has
+    // no corresponding input pixel.
+    NDArray<ssize_t, 2> combined({total_rows, ncols}, -1);
+
+    // Copy maps into the combined output.
+    auto copy_map = [&](auto const &source, ssize_t destination_row) {
+        const ssize_t nrows = source.map.shape(0);
+
+        for (ssize_t row = 0; row < nrows; ++row) {
+            for (ssize_t col = 0; col < ncols; ++col) {
+                combined(destination_row + row, col) = source.map(row, col);
+            }
+        }
+    };
+
+    const ssize_t first_row = 0;
+    const ssize_t second_row = first.map.shape(0) + gap_rows;
+
+    copy_map(first, first_row);
+    copy_map(second, second_row);
+
+    // Physical pixel ROI covered by both groups.
+    InclusiveROI effective_roi = first.effective_roi;
+
+    effective_roi.xmin =
+        std::min(effective_roi.xmin, second.effective_roi.xmin);
+    effective_roi.xmax =
+        std::max(effective_roi.xmax, second.effective_roi.xmax);
+    effective_roi.ymin =
+        std::min(effective_roi.ymin, second.effective_roi.ymin);
+    effective_roi.ymax =
+        std::max(effective_roi.ymax, second.effective_roi.ymax);
+
+    return {.map = std::move(combined), .effective_roi = effective_roi};
+}
+} // namespace detail
+
+// ============================================================
+// Public generators
+// ============================================================
+
 /************************************
  * Single chip, multi-pitch, iLGAD
  ************************************/
-inline defs::StrixelGroupToPixelMap
-jungfrau_ilgad_singlechip_25um_strixel_map(InclusiveROI rx_roi,
-                                           defs::SensorModulePlacement placement,
-                                           defs::BondShift bs = {0, 0}) {
+inline defs::StrixelGroupToPixelMap jungfrau_ilgad_singlechip_25um_strixel_map(
+    InclusiveROI rx_roi, defs::SensorModulePlacement placement,
+    defs::BondShift bs = {0, 0}) {
     std::cout << " === JUNGFRAU iLGAD SINGLE-CHIP 25um PITCH === \n";
     return algo::strixel_to_pixel_map(config::jungfrau::SingleChipMP_iLGAD_P25,
                                       config::jungfrau::SingleChipMP_iLGAD_pix,
                                       placement, rx_roi, bs);
 }
 
-inline defs::StrixelGroupToPixelMap
-jungfrau_ilgad_singlechip_15um_strixel_map(InclusiveROI rx_roi,
-                                           defs::SensorModulePlacement placement,
-                                           defs::BondShift bs = {0, 0}) {
+inline defs::StrixelGroupToPixelMap jungfrau_ilgad_singlechip_15um_strixel_map(
+    InclusiveROI rx_roi, defs::SensorModulePlacement placement,
+    defs::BondShift bs = {0, 0}) {
     std::cout << " === JUNGFRAU iLGAD SINGLE-CHIP 15um PITCH === \n";
     return algo::strixel_to_pixel_map(config::jungfrau::SingleChipMP_iLGAD_P15,
                                       config::jungfrau::SingleChipMP_iLGAD_pix,
                                       placement, rx_roi, bs);
 };
 
-inline defs::StrixelGroupToPixelMap
-jungfrau_ilgad_singlechip_18um_strixel_map(InclusiveROI rx_roi,
-                                           defs::SensorModulePlacement placement,
-                                           defs::BondShift bs = {0, 0}) {
+inline defs::StrixelGroupToPixelMap jungfrau_ilgad_singlechip_18um_strixel_map(
+    InclusiveROI rx_roi, defs::SensorModulePlacement placement,
+    defs::BondShift bs = {0, 0}) {
     std::cout << " === JUNGFRAU iLGAD SINGLE-CHIP 18.75um PITCH === \n";
     return algo::strixel_to_pixel_map(config::jungfrau::SingleChipMP_iLGAD_P18,
                                       config::jungfrau::SingleChipMP_iLGAD_pix,
@@ -54,12 +155,9 @@ jungfrau_ilgad_singlechip_multipitch_strixel_maps(InclusiveROI rx_roi,
         // std::optional<InclusiveROI> sensor_placement)
         throw std::runtime_error("Invalid sensor placement.");
     }
-    defs::SensorConfig configs{config::jungfrau::SingleChipMP_iLGAD_pix,
-                               {config::jungfrau::SingleChipMP_iLGAD_P25,
-                                config::jungfrau::SingleChipMP_iLGAD_P15,
-                                config::jungfrau::SingleChipMP_iLGAD_P18}};
 
-    return algo::strixel_to_pixel_maps(configs, placement, rx_roi, bs);
+    return algo::strixel_to_pixel_maps(config::jungfrau::SingleChipMP_iLGAD,
+                                       placement, rx_roi, bs);
 };
 
 // More generic overload
@@ -67,12 +165,9 @@ inline std::vector<defs::StrixelGroupToPixelMap>
 jungfrau_ilgad_singlechip_multipitch_strixel_maps(
     InclusiveROI rx_roi, defs::SensorModulePlacement placement,
     defs::BondShift bs = {0, 0}) {
-    defs::SensorConfig configs{config::jungfrau::SingleChipMP_iLGAD_pix,
-                               {config::jungfrau::SingleChipMP_iLGAD_P25,
-                                config::jungfrau::SingleChipMP_iLGAD_P15,
-                                config::jungfrau::SingleChipMP_iLGAD_P18}};
 
-    return algo::strixel_to_pixel_maps(configs, placement, rx_roi, bs);
+    return algo::strixel_to_pixel_maps(config::jungfrau::SingleChipMP_iLGAD,
+                                       placement, rx_roi, bs);
 };
 
 /************************************
@@ -124,33 +219,26 @@ jungfrau_tew_singlechip_multipitch_strixel_maps(InclusiveROI rx_roi,
         // std::optional<InclusiveROI> sensor_placement)
         throw std::runtime_error("Invalid sensor placement.");
     }
-    defs::SensorConfig configs{config::jungfrau::SingleChipMP_TEW_pix,
-                               {config::jungfrau::SingleChipMP_TEW_P25,
-                                config::jungfrau::SingleChipMP_TEW_P15,
-                                config::jungfrau::SingleChipMP_TEW_P18}};
 
-    return algo::strixel_to_pixel_maps(configs, placement, rx_roi, bs);
+    return algo::strixel_to_pixel_maps(config::jungfrau::SingleChipMP_TEW,
+                                       placement, rx_roi, bs);
 };
 
 // More generic overload
 inline std::vector<defs::StrixelGroupToPixelMap>
-jungfrau_tew_singlechip_multipitch_strixel_maps(InclusiveROI rx_roi,
-                                                defs::SensorModulePlacement placement,
-                                                defs::BondShift bs = {0, 0}) {
-    defs::SensorConfig configs{config::jungfrau::SingleChipMP_TEW_pix,
-                               {config::jungfrau::SingleChipMP_TEW_P25,
-                                config::jungfrau::SingleChipMP_TEW_P15,
-                                config::jungfrau::SingleChipMP_TEW_P18}};
+jungfrau_tew_singlechip_multipitch_strixel_maps(
+    InclusiveROI rx_roi, defs::SensorModulePlacement placement,
+    defs::BondShift bs = {0, 0}) {
 
-    return algo::strixel_to_pixel_maps(configs, placement, rx_roi, bs);
+    return algo::strixel_to_pixel_maps(config::jungfrau::SingleChipMP_TEW,
+                                       placement, rx_roi, bs);
 };
 
 /************************************
  * Quad, 25 um, iLGAD
  ************************************/
-inline defs::StrixelGroupToPixelMap
-jungfrau_ilgad_quadbottom_25um_strixel_map(InclusiveROI rx_roi,
-                                           defs::SensorModulePlacement placement) {
+inline defs::StrixelGroupToPixelMap jungfrau_ilgad_quadbottom_25um_strixel_map(
+    InclusiveROI rx_roi, defs::SensorModulePlacement placement) {
     return algo::strixel_to_pixel_map(config::jungfrau::Quad_iLGAD_bottomhalf,
                                       config::jungfrau::Quad_iLGAD_pix,
                                       placement, rx_roi);
@@ -168,19 +256,16 @@ inline std::vector<defs::StrixelGroupToPixelMap>
 jungfrau_ilgad_quad_25um_strixel_maps(InclusiveROI rx_roi,
                                       defs::SensorModulePlacement placement) {
 
-    defs::SensorConfig configs{config::jungfrau::Quad_iLGAD_pix,
-                               {config::jungfrau::Quad_iLGAD_bottomhalf,
-                                config::jungfrau::Quad_iLGAD_tophalf}};
-
-    return algo::strixel_to_pixel_maps(configs, placement, rx_roi);
+    return algo::strixel_to_pixel_maps(config::jungfrau::Quad_iLGAD, placement,
+                                       rx_roi);
 }
 
 inline defs::StrixelGroupToPixelMap
 jungfrau_ilgad_quad_25um_strixel_map(InclusiveROI rx_roi,
                                      defs::SensorModulePlacement placement,
                                      defs::BondShift bs = {0, 0}) {
-    std::vector<int> gap_rows{12, 0};
     auto maps = jungfrau_ilgad_quad_25um_strixel_maps(rx_roi, placement);
-    return algo::combine_maps(maps, gap_rows);
+    return detail::combine_group_maps(
+        maps[0], maps[1], config::jungfrau::Quad_iLGAD_strixel_gap_rows);
 }
-} // namespace aare::remap::generate
+} // namespace aare::remap::generate

src/RemapAlgorithm.cpp

@@ -4,23 +4,8 @@
 
 namespace aare::remap::algo {
 
-// Is it better to pass defs::SensorGroupConfig const& and return a copy?
-// Better not to, use shift_rotate_roi instead
-void apply_rotation_shift(defs::GroupConfig &cfg,
-                          defs::SensorPixelGeometry const &pixel,
-                          defs::BondShift bond_shift, defs::Rotation rot) {
-    // Apply physical transforms
-    if (bond_shift.x != 0 || bond_shift.y != 0)
-        cfg.placement_on_sensor = aare::inclusiveroi::geom::translate(
-            cfg.placement_on_sensor, bond_shift.x, bond_shift.y);
-
-    if (rot == defs::Rotation::Rotate180)
-        cfg.placement_on_sensor = aare::inclusiveroi::geom::mirrorXY(
-            cfg.placement_on_sensor, pixel.num_pix_x, pixel.num_pix_y);
-}
-
 /**
- * Apply physical transformations to a sensor-local ROI.
+ * @brief Apply physical transformations to a sensor-local ROI.
  *
  * IMPORTANT:
  * Bond shifts are applied before rotation.
@@ -94,7 +79,7 @@ strixel_to_pixel_map(defs::GroupConfig const &group_config,
     // -- 3) Compute effective ROI = intersection( roi_user, roi_group )
     InclusiveROI eff = inclusiveroi::geom::intersect(roi_user_local, roi_group);
     if (eff.xmax < eff.xmin || eff.ymax < eff.ymin) {
-        return {-1, 0.0, InclusiveROI::emptyROI(), {}}; // empty
+        return {{}, InclusiveROI::emptyROI()}; // empty
     }
 
     // DEBUG
@@ -131,7 +116,7 @@ strixel_to_pixel_map(defs::GroupConfig const &group_config,
     }
 
     if (min_row_strx > max_row_strx) {
-        return {multiplicity, pitch, eff, {}}; // nothing mapped
+        return {{}, eff}; // nothing mapped
     }
 
     // Now from the found bounds of the strixel grid, we define the space to
@@ -178,7 +163,7 @@ strixel_to_pixel_map(defs::GroupConfig const &group_config,
         }
     }
 
-    return {multiplicity, pitch, eff, map};
+    return {map, eff};
 };
 
 std::vector<defs::StrixelGroupToPixelMap>
@@ -199,58 +184,4 @@ strixel_to_pixel_maps(defs::SensorConfig const &sensor_config,
     return maps;
 }
 
-defs::StrixelGroupToPixelMap
-combine_maps(std::vector<defs::StrixelGroupToPixelMap> const &maps,
-             std::vector<int> const &gaps) {
-
-    if (maps.size() != gaps.size()) {
-        throw std::logic_error("Gaps provided are inclompatible with "
-                               "number of maps to combine. Number of gaps must "
-                               "be equal the number of maps.");
-    }
-
-    auto [_, global_cols] = maps[0].map.shape();
-    int global_rows = 0;
-    int const m = maps[0].multiplicity;
-    double const p = maps[0].pitch_um;
-    auto placement_on_sensor = maps[0].placement_on_sensor;
-    std::vector<int> offsets(maps.size());
-    for (size_t i = 0; i < maps.size(); ++i) {
-
-        offsets[i] = global_rows;
-
-        if (maps[i].multiplicity != m) {
-            throw std::logic_error("Maps contain incompatible multiplicities.");
-        }
-
-        if (maps[i].pitch_um != p) {
-            throw std::logic_error("Maps contain incompatible pitches.");
-        }
-
-        auto [temp_rows, temp_cols] = maps[i].map.shape();
-        global_cols = std::max(global_cols, temp_cols);
-        global_rows = global_rows + temp_rows + gaps[i];
-    }
-
-    NDArray<ssize_t, 2> map({global_rows, global_cols}, -1);
-
-    for (size_t i = 0; i < maps.size(); ++i) {
-        auto [rows, cols] = maps[i].map.shape();
-
-        // DEBUG
-        std::cout << "DEBUG: Row offset: i = " << i
-                  << ", offset: " << offsets[i] << '\n';
-
-        for (ssize_t r = 0; r < rows; ++r) {
-            for (ssize_t c = 0; c < cols; ++c) {
-                map(offsets[i] + r, c) = maps[i].map(r, c);
-            }
-        }
-    }
-
-    // For combined maps, placement_on_sensor in principle is no longer correct
-    // TODO: Decide how to handle this!
-    return {m, p, placement_on_sensor, map};
-}
-
 } // namespace aare::remap::algo