Dev/strixels/remap_simple by vhinger182 · Pull Request #286 · slsdetectorgroup/aare

vhinger182 · 2026-03-10T16:58:56Z

Refactor of PR #275, branching off dev/strixels/remap
The structure has been simplified.

Basic structure:

defs -> pure data structures (geometry, config structs, placement, remapping result)
config -> hardcoded sensor definitions (e.g. Jungfrau iLGAD multipitch, 25um pitch group, ...)
algo -> generic remapping algorithms
generate -> user-facing convenience functions (i.e. generate a map for Jungfrau iLGAD multipitch sensor 25um pitch group)

So far, only a few basic sensor configurations are implemented. But in principle, it should be straight forward to implement a new one by

Adding geometry definitions in config
Adding generate function

We could also discuss a further simplification making the sensor definitions types, i.e. instead of

auto map = jungfrau_ilgad_singlechip_25um_strixel_map(roi, sensor_placement);

we template on the sensor type

auto map = generate_maps<Jungfrau_iLGAD_MP25>(roi, sensor_placement);

Then adding a new sensor just requires a new type definition.

…empty()

AliceMazzoleni99 · 2026-04-09T08:28:17Z

+inline constexpr defs::SensorGroupConfig SingleChipMP_iLGAD_P25{
+    .pixel = SingleChipMP_iLGAD_pix,
+    .strixel = StrxP25,
+    .placement_on_sensor = {SingleChipMP_iLGAD_pix.guardring.x + 1,


not sure does indexing always start at zero? e.g. guardring of 9 pixels then roi starts at pixel 9 for zero indexing? Maybe I misunderstand something.

AliceMazzoleni99 · 2026-04-09T08:31:32Z

+                            (SingleChipMP_iLGAD_pix.num_pix_y / 4) * 2 - 1}};
+
+inline constexpr defs::SensorGroupConfig SingleChipMP_iLGAD_P18{
+    .pixel = SingleChipMP_iLGAD_pix,


Maybe this pixel grid information should belong to struct SensorConfig?

AliceMazzoleni99 · 2026-04-09T08:34:53Z

+
+/**************************************
+ * Legacy for reference, to be deleted
+ **************************************/


yes delete all of this

AliceMazzoleni99 · 2026-04-09T08:57:50Z

+static inline std::string toString(defs::Rotation);
+static inline std::string toString(defs::SensorPixelGeometry const &g);
+static inline std::string toString(defs::SensorStrixelGeometry const &g);
+static inline std::string toString(defs::SensorGroupConfig const &c);


Are the conversions every used? Or only used for debugging? You could easily use , It doesnt hurt to keep it but I would move everything to to_string.hpp and maybe use lower case to_string to keep consistent with the rest of the code.

AliceMazzoleni99 · 2026-04-09T09:11:41Z

+    InclusiveROI const &user_roi, defs::BondShift bond_shift = {0, 0});
+
+std::vector<defs::StrixelGroupToPixelMap> strixel_to_pixel_maps(
+    defs::SensorConfig const &, std::vector<defs::SensorPlacement> const &,


why are there multiple sensorplacements? I thought SensorConfig is unique for one sensor and thus only one placement on the module?

AliceMazzoleni99 · 2026-04-09T09:12:20Z

+    std::vector<SensorGroupConfig> group_configs;
+};
+
+// Define location and orientation of a sensor on the module


maybe SensorModulePlacement is more explicit?

AliceMazzoleni99 · 2026-04-09T09:28:13Z

+                output(row, col) = value;
+                // output(row, col) = static_cast<T>(input[flat_index]);
+            } else {
+                output(row, col) = static_cast<T>(0); // or nan?


I would throw a runtime error?

AliceMazzoleni99 · 2026-04-09T09:29:38Z

+ * \param output Remapped array
+ */
+template <typename T>
+void ApplyRemap(NDView<T, 2> const &input, NDArray<ssize_t, 2> const &order_map,


I would pass a view instead of reference to NDArray

Maybe its better to directly return the remapped array than passing?

AliceMazzoleni99 · 2026-04-09T09:32:45Z

+ *
+ * \param input Original array
+ * \param order_map Rule for remapping
+ * \param output Remapped array


We could move this function to PixelMap.hpp

AliceMazzoleni99 · 2026-04-09T09:35:54Z

+template <typename T>
+void ApplyRemap(NDView<T, 2> const &input, NDArray<ssize_t, 2> const &order_map,
+                NDArray<T, 2> &output) {
+    for (size_t row = 0; row < order_map.shape(0); ++row) {


compiler warning: use ssize_t

also for col

AliceMazzoleni99 · 2026-04-09T09:47:08Z

+
+// Is it better to pass defs::SensorGroupConfig const& and return a copy?
+void apply_rotation_shift(defs::SensorGroupConfig &cfg,
+                          defs::BondShift bond_shift, defs::Rotation rot) {


yes everything should be const here

AliceMazzoleni99 · 2026-04-09T09:51:34Z

+
+    // Define mod ordering (Normal or Inverse)
+    std::vector<int> mods(multiplicity);
+    for (int i = 0; i < multiplicity; i++)


maybe use std::iota

AliceMazzoleni99 · 2026-04-09T09:53:31Z

+    // -- 1) Transform user roi (rx_roi) into sensor-local coordinates
+    InclusiveROI roi_user_local =
+        inclusiveroi::geom::alignROIs(roi_user, placement.placement_on_module);
+    std::cout << "Transformed user ROI: " << roi_user_local << std::endl;


remove std::cout or add LOG(logDEBUG) <<

AliceMazzoleni99 · 2026-04-09T09:54:00Z

+    std::cout << "Transformed user ROI: " << roi_user_local << std::endl;
+
+    // DEBUG
+    std::cout << "DEBUG: Group ROI before transformation (as in global config)"


also replace by LOG(logDEBUG)

AliceMazzoleni99 · 2026-04-09T10:13:38Z

+// Possibly, this may need to be moved to a dedicated "remapping" class
+struct StrixelGroupToPixelMap {
+    int multiplicity;
+    double pitch_um;


yes I would remove these things. Is placement_on_sensor the same ans in SensorGroupConfig?

AliceMazzoleni99 · 2026-04-09T10:18:17Z

+             std::vector<int> const &gaps) {
+
+    if (maps.size() != gaps.size()) {
+        throw std::logic_error("Gaps provided are inclompatible with "


typo: incompatible

AliceMazzoleni99 · 2026-04-09T10:23:30Z

+
+        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];


not sure what gaps[i] should be due to placement on sensor?

AliceMazzoleni99 · 2026-04-09T10:24:08Z

+        auto [rows, cols] = maps[i].map.shape();
+
+        // DEBUG
+        std::cout << "DEBUG: Row offset: i = " << i


again LOG(logDEBUG) <<

AliceMazzoleni99 · 2026-04-09T10:26:21Z

+        global_rows = global_rows + temp_rows + gaps[i];
+    }
+
+    NDArray<ssize_t, 2> map({global_rows, global_cols}, -1);


skip -1 in Applyremap?

AliceMazzoleni99 · 2026-04-09T10:30:35Z

@@ -0,0 +1,87 @@
+#pragma once
+


maybe better to remap the file to StrixelPixelMap.hpp

maybe its better to rename all files to StrixelPixelRemap... makes it more explicit what it is

AliceMazzoleni99 · 2026-04-09T11:45:20Z

+};
+
+inline std::vector<defs::StrixelGroupToPixelMap>
+jungfrau_ilgad_singlechip_multipitch_strixel_maps(InclusiveROI rx_roi,


I would make this independant of chip_id and have a constexpr SensorConfig jungfrau_ilgad{{config::jungfrau::SingleChipMP_iLGAD_P25,
config::jungfrau::SingleChipMP_iLGAD_P15,
config::jungfrau::SingleChipMP_iLGAD_P18}}

AliceMazzoleni99 · 2026-04-09T11:46:47Z

+    std::vector<defs::SensorPlacement> placements(configs.group_configs.size(),
+                                                  placement);
+
+    placements[1].rotation = algo::flip(placement.rotation);


what does flip do?

AliceMazzoleni99 · 2026-04-09T11:49:54Z

+
+    return algo::strixel_to_pixel_maps(configs, placements, rx_roi);
+}
+


functions definitly need to be commented. Maybe we only need to define one jungfrau pixel map with the combined maps? If someone wants the sensor group map one has still the freedom to calculate it.

AliceMazzoleni99

Added a few minor comments. But it needs to be documented more precisely. Every function in particular the functions in RemapAlgorithm and RemapGenerate need to have a proper docstring e.g.
`
/** @brief short explanation

@param define all parameters
@return what does it return
/
`
and all the struct members should also have a /// @brief short member explanation
Otherwise it looks very good.

vhinger182 added 30 commits February 24, 2026 19:33

Add first remapping draft

bb237ff

Fix libc++ compilation error

32eb686

Remove outdated FIXME comment

95f05c6

Stylistic rewrite to make read-only intent clear

ee2dc56

ROI conversion helpers

71a7024

Merge branch 'main' into dev/strixels/remap

e31c858

Rename for clarity

fba4aa7

Merge branch 'main' into dev/strixels/remap

1f12328

Prevent acces and throw of m_rois.value() in if condition if m_rois->…

f9a74a3

…empty()

Merge branch 'fix/ROI_bad_optional_access' into dev/strixels/remap

d6127f3

Merge branch 'main' into fix/ROI_bad_optional_access

e445630

Specify throw if empty

9f5c1a7

Merge branch 'fix/ROI_bad_optional_access' into dev/strixels/remap

586012f

Merge branch 'main' into dev/strixels/remap

be5b09d

SImplify StrixelSensorConfig (1st step)

33c0c55

Add missing simplification steps

1ec11e4

Adapt namespaces

495da9b

Create empty StrixelSensorConfig class

fe1b818

Draft core remapping definitions

ac75234

Add map structure

86db6c3

Add more map structure specifiers

dc963f1

Draft remap algorithm

f05c412

Implement general remapping (first draft)

166c0e0

Add ApplyRemap

d4091d5

Add formatters

5cc2b9f

Add hardcoded Jungfrau config structures

e00b1d2

Adapt for renaming of legacy namespace

89e5593

Merge branch 'main' into dev/strixels/remap_simple

09bfcbf

Add rotation and bond_shift

02045c5

Add generation functions for Jungfrau multipitch strixel

db5c453

AliceMazzoleni99 reviewed Apr 9, 2026

View reviewed changes

vhinger182 added 6 commits May 22, 2026 18:12

Merge branch 'main' into dev/strixels/remap_simple

47ebbd2

Add more generators

4b8d71a

Clarify layout with comments and printouts

d7dd3a0

Merge branch 'main' into dev/strixels/remap_simple

d87e834

Fix comment

414d268

Merge branch 'main' into dev/strixels/remap_simple

e70392a


		return algo::strixel_to_pixel_maps(configs, placements, rx_roi);
		}

Conversation

vhinger182 commented Mar 10, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AliceMazzoleni99 Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AliceMazzoleni99 Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AliceMazzoleni99 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

AliceMazzoleni99 Apr 9, 2026 •

edited

Loading

AliceMazzoleni99 Apr 9, 2026 •

edited

Loading