-
Notifications
You must be signed in to change notification settings - Fork 3
Enhancements for Becker & Hickl SPCM Support #48
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: development
Are you sure you want to change the base?
Conversation
- Implement parse_bh_set_file() to parse BH .set files - Implement detect_frame1_extra_line() to detect Frame 1 initialization line anomaly - Integrate helpers into tttrlib Python namespace via tttrlib.i - Add unit tests in test/test_bh_helpers.py
- Add use_pixel_markers and marker_pixel parameters to CLSMSettings - Implement logic in CLSMImage::fill() to bin photons using pixel markers - Expose new settings to Python interface by making CLSMSettings members public
- Added get_adjusted_frame_markers() to handle extra initialization line - Added prepare_bh_clsm_settings() convenience function - Added unit tests in test/test_bh_helpers.py
…n CLSMImage constructor
…additions Reverts formatting changes (quote style, blank lines, indentation) while preserving the use_pixel_markers and marker_pixel parameter additions.
…ect_bh_frame1_extra_line
…erflow guard in create_frames
- Remove ext/python/bh_helpers.py (functionality folded into core C++) - Remove bh_helpers reference from SWIG interface (tttrlib.i) - Remove test/test_bh_helpers.py (tests deprecated module) - Fix BH .set file parsing to use correct '#SP [KEY,TYPE,VALUE]' format Reverts the non-functional changes from commit 04698e1 while keeping the improved BH .set file parsing logic.
Align formatting with upstream and remove trailing whitespace; no behavioral changes.
Implement bh_fix_missing_markers to detect orphan data at the end of BH SPC-130 files. If data exists but markers are missing (e.g. due to truncated recording), synthetic pixel, line, and frame markers are appended to allow recovery of the final frame.
…markers" This reverts commit 0e47aea.
|
Data used for testing: https://doi.org/10.5281/zenodo.18326776 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR enhances tttrlib's support for Becker & Hickl SPC data files recorded by SPCM software. It adds automated metadata extraction from .set files, pixel marker-based binning for precise spatial localization, dimension inference from markers, and recovery of truncated recordings.
Changes:
- Automated parsing of BH .set configuration files to extract image dimensions and pixel clock settings
- Pixel marker-based binning support for BH data, enabling precise photon-to-pixel assignment
- Dimension inference logic to automatically determine image size from TTTR markers when metadata is unavailable
- Truncated recording recovery to handle incomplete final frames in BH SPC-130 data
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| test/test_CLSM_bh_markers.py | New integration tests validating pixel marker binning, flyback handling, and truncated recording recovery |
| src/TTTRHeader.cpp | Implements BH .set file parser to extract imaging parameters |
| include/TTTRHeader.h | Adds declaration for read_bh_set_file() method |
| src/TTTR.cpp | Auto-discovers and parses .set files when loading BH SPC-130 containers |
| include/TTTRRecordReader.h | Adds marker record processing for BH SPC-130 format with proper bit field interpretation |
| src/CLSMImage.cpp | Implements BH-specific defaults, dimension inference, frame 1 adjustment, truncated line recovery, and pixel marker-based binning |
| include/CLSMImage.h | Adds CLSM_BH_SPC130 reading routine, pixel marker settings, and detect_bh_frame1_extra_line() method |
| ext/python/CLSMImage.py | Adds Python interface support for pixel markers and BH_SPC130 reading routine with consistent defaults |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| friend class CLSMImage; | ||
|
|
||
| protected: | ||
| public: |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing CLSMSettings members from protected to public changes the class interface. This could expose internal state that was previously encapsulated. Consider whether all these members need to be public or if only specific accessors should be provided.
| public: | |
| protected: |
src/CLSMImage.cpp
Outdated
| } | ||
|
|
||
| // Frame 1 anomaly: one extra initialization line marker | ||
| return (n_lines_f1 == n_lines_f2 + 1); |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition checks if n_lines_f1 == n_lines_f2 + 1, which detects Frame 1 having exactly one more line marker than Frame 2. However, if Frame 2 happens to be truncated or incomplete while Frame 1 is correct, this would incorrectly flag Frame 1 as having an anomaly. Consider checking at least Frame 3 as well, or comparing against multiple subsequent frames to ensure the pattern is consistent.
src/CLSMImage.cpp
Outdated
| // Task 7: BH SPC-130 Frame 1 adjustment | ||
| if (reading_routine == CLSM_BH_SPC130 && frame_edges.size() >= 2) { | ||
| int frame_m = marker_frame.empty() ? 4 : marker_frame[0]; | ||
| int line_m = 2; // BH default line marker |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code hardcodes the line marker as int line_m = 2 but this assumes the user hasn't customized the line marker channel. If a user sets a different marker_line_start value in their settings, this hardcoded value would be incorrect. Consider using the actual settings value or passing it as a parameter.
| int line_m = 2; // BH default line marker | |
| int line_m = (tttr.marker_line_start > 0) ? tttr.marker_line_start : 2; // Use configured BH line marker, default to 2 |
src/TTTR.cpp
Outdated
| if (filename.size() >= 4) { | ||
| std::string ext = filename.substr(filename.size() - 4); | ||
| // Case-insensitive check for .spc extension | ||
| std::transform(ext.begin(), ext.end(), ext.begin(), ::tolower); |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code uses std::transform with ::tolower for case-insensitive extension comparison, but ::tolower behavior is locale-dependent and undefined for char values outside the range of unsigned char. Consider using std::tolower from <locale> with proper casting, or compare against both ".spc" and ".SPC" explicitly.
| signed char marker_start_sc = static_cast<signed char>(marker_line_start); | ||
| signed char marker_stop_sc = static_cast<signed char>(marker_line_stop); | ||
| signed char marker_event_sc = static_cast<signed char>(marker_event_type); | ||
| const signed char* routing_channels = tttr->routing_channels; | ||
| const signed char* event_types = tttr->event_types; | ||
|
|
||
| for (int idx : line_edges) { | ||
| if (event_types[idx] == marker_event_sc) { | ||
| if (routing_channels[idx] == marker_start_sc) start_count++; | ||
| else if (routing_channels[idx] == marker_stop_sc) stop_count++; | ||
| } | ||
| } |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code counts start and stop markers, but directly uses signed char comparison in the loop without considering that marker channels could potentially be outside signed char range. While routing_channels is stored as signed char, the function parameters are int, which could cause issues if large marker values are passed. Consider validating that marker values fit in signed char range or using a consistent type.
ext/python/CLSMImage.py
Outdated
|
|
||
| elif reading_routine == 'BH_SPC130': | ||
| # BH SPC-130 specific defaults | ||
| # These mirror the C++ defaults in CLSMImage.cpp:380-396 |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment references CLSMImage.cpp lines 380-396, but the actual C++ code for BH SPC-130 defaults spans a much larger range (lines 379-507 in the diff). This comment is misleading and should be updated to either remove the line reference or accurately reflect where the defaults are applied.
| # These mirror the C++ defaults in CLSMImage.cpp:380-396 | |
| # These mirror the BH SPC-130 defaults implemented in CLSMImage.cpp. |
| signed char marker_start_sc = static_cast<signed char>(marker_line_start); | ||
| signed char marker_stop_sc = static_cast<signed char>(marker_line_stop); | ||
| signed char marker_event_sc = static_cast<signed char>(marker_event_type); | ||
| const signed char* routing_channels = tttr->routing_channels; | ||
| const signed char* event_types = tttr->event_types; |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variables marker_start_sc, marker_stop_sc, and marker_event_sc are redeclared inside the single-marker mode block, shadowing the variables declared earlier at lines 778-782. This creates unnecessary duplication and could be confusing. The existing variables should be reused instead of redeclaring them.
| signed char marker_start_sc = static_cast<signed char>(marker_line_start); | |
| signed char marker_stop_sc = static_cast<signed char>(marker_line_stop); | |
| signed char marker_event_sc = static_cast<signed char>(marker_event_type); | |
| const signed char* routing_channels = tttr->routing_channels; | |
| const signed char* event_types = tttr->event_types; |
| } catch (...) { | ||
| // Ignore lines with invalid integer values | ||
| } | ||
| } |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The try-catch block catches all exceptions with catch (...) but silently ignores them. This could hide unexpected errors like bad_alloc or other critical failures. Consider at minimum logging ignored exceptions in verbose mode, or catching only specific expected exceptions like std::invalid_argument and std::out_of_range from std::stoi.
| } catch (...) { | ||
| // Header parsing failed, continue with settings as-is | ||
| if (is_verbose()) { | ||
| std::clog << "-- BH: Could not read header metadata" << std::endl; | ||
| } | ||
| } |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to TTTRHeader.cpp, this catch-all exception handler silently ignores all exceptions during header parsing. This could hide important errors. Consider catching specific exceptions or at least logging the error in verbose mode.
src/CLSMImage.cpp
Outdated
| std::clog << "-- CREATE_FRAMES" << std::endl; | ||
| std::cout << "-- Creating " << frame_edges.size() << " frames: " << std::flush; | ||
| } | ||
| if (frame_edges.size() < 2) { |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The guard condition checks if frame_edges.size() < 2, but the code then proceeds to iterate with for (size_t i = 0; i < frame_edges.size() - 1; i++). While the guard prevents issues when frame_edges is empty or has only 1 element, it would be more explicit to check frame_edges.size() <= 1 to make the logic clearer.
| if (frame_edges.size() < 2) { | |
| if (frame_edges.size() <= 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a188a4d6ce
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Read n_pixel_per_line from header if not explicitly binned by user | ||
| if (this->settings.n_pixel_per_line <= 1) { | ||
| auto pixX_tag = TTTRHeader::get_tag(json, "ImgHdr_PixX"); | ||
| if (!pixX_tag.is_null() && pixX_tag.contains("value")) { | ||
| this->settings.n_pixel_per_line = pixX_tag["value"].get<int>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Respect explicit n_pixel_per_line=1 for BH data
The new BH defaulting logic treats n_pixel_per_line <= 1 as “unset” and overwrites it from the header. This conflicts with the documented behavior of CLSMSettings (“auto‐determine only when set to 0”) and makes it impossible to intentionally bin a line to a single pixel when using CLSM_BH_SPC130. In practice, callers passing n_pixel_per_line=1 now get silently overridden to the header/inferred size, changing image shape unexpectedly. Consider only auto-filling when the value is 0 (or track an explicit/implicit flag) so a deliberate 1‑pixel bin is honored.
Useful? React with 👍 / 👎.
- Treat n_pixel_per_line=0 as auto-detect (honor explicit 1-pixel lines) - Use configured marker_line_start instead of hardcoded 2 in BH correction - Improve BH Frame 1 detection by verifying baseline against Frame 2 and 3 - Fallback to settings.n_lines for Frame 1 detection when < 3 frames exist - Clarify guard condition in create_frames
- Fix locale-dependent tolower UB in TTTR.cpp using safe ASCII lambda - Add verbose logging for exceptions in BH .set file parsing
- Define CLSM_MARKER_NO_STOP constant (255) for start-only markers - Remove variable shadowing in get_line_edges - Update Python comments to reflect C++ implementation and new constant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for (size_t i = 0; i < n_events; ++i) { | ||
| if (event_types[i] == marker_event_type) { | ||
| if (routing_channels[i] == frame_marker) { | ||
| frame_marker_positions.push_back(static_cast<int>(i)); | ||
| if (frame_marker_positions.size() >= 4) break; | ||
| } | ||
| } |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the loop at line 912, event_types[i] is a signed char and is being compared directly to marker_event_type which is an int parameter. Similarly, routing_channels[i] is a signed char being compared to frame_marker which is an int. While this will work due to implicit conversion, it would be more consistent and efficient to cast the int parameters to signed char once before the loop (similar to what is done in count_line_markers_in_range at lines 883-884).
| constexpr int CLSM_MARKER_NO_STOP = 255; | ||
|
|
||
|
|
||
| /// Different types of distances between two accessible volumes |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment on line 29 says "Different types of distances between two accessible volumes" but this enum actually defines reading routines for different CLSM hardware types. This is misleading documentation that was already present, but since the enum is being modified, the comment should be corrected to accurately describe what ReadingRoutine represents.
| /// Different types of distances between two accessible volumes | |
| /// Reading routines corresponding to different CLSM hardware types |
| signed char marker_start_sc = static_cast<signed char>(marker_line_start); | ||
| signed char marker_stop_sc = static_cast<signed char>(marker_line_stop); | ||
| signed char marker_event_sc = static_cast<signed char>(marker_event_type); | ||
| const signed char* routing_channels = tttr->routing_channels; | ||
| const signed char* event_types = tttr->event_types; |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Variables marker_start_sc, marker_stop_sc, marker_event_sc, routing_channels, and event_types are being redeclared here, but they were already declared earlier in the function (lines 781-786, 777-778). The redeclarations shadow the earlier variables and are redundant since the earlier declarations are still in scope.
| signed char marker_start_sc = static_cast<signed char>(marker_line_start); | |
| signed char marker_stop_sc = static_cast<signed char>(marker_line_stop); | |
| signed char marker_event_sc = static_cast<signed char>(marker_event_type); | |
| const signed char* routing_channels = tttr->routing_channels; | |
| const signed char* event_types = tttr->event_types; |
Introduction
Hi, thank you for this wonderful library! I was trying to read some spc files recorded by BH's SPCM software, and realized that photon markers weren't being read by tttrlib. So this PR introduces improvements for Becker & Hickl SPC data support in
tttrlib. It adds automated metadata extraction, support for marker-based binning, and robust dimension inference.The implementation was done with reference to PAM's implementation at https://gitlab.com/PAM-PIE/PAM, though somewhat improved to better match data exported via vendor software.
I tested with intensity exports from my own data and they match the vendor software exports exactly, except for a) a shift of 1 pixel in Y and b) slight deviations in some pixels (+- 1 or 2 photons). For a), BH's image has a dark line at the end of the image, so this PR may in fact be a marginally more accurate representation of the data. b) might be because of binning issues or because of different microtime ranges.
Currently the reading routines are named 'BH_SPC130'/'CLSM_BH_SPC130', but I think more accurately it works with SPC-140 and 150 as well (I am using 150), as long as they are recorded by the SPCM64 software from BH. Perhaps they can be renamed to more accurately reflect the intended use case.
Anyway, AI summary of the changes below:
Key Changes
1. Header and Metadata Enhancement
read_bh_set_file()inTTTRHeaderto parse BH configuration files. This allows automatic extraction of image dimensions (SP_IMG_X,SP_IMG_Y) and pixel clock status.TTTR::read_file()now automatically searches for and parses a corresponding.setfile when loading BH SPC-130 data, ensuring metadata is available without manual intervention.2. CLSM Imaging Improvements (CLSMImage) for .spc files recorded by BH SPCM:
detect_bh_frame1_extra_line()and associated logic to handle an extra line marker in the beginning of the first frame.3. Core Logic & Cleanup
CLSM_BH_SPC130to theReadingRoutineenum to explicitly handle BH-specific CLSM workflows.bh_helpers.pyutility as its functionality is now integrated into the core library.4. Testing
test/test_CLSM_bh_markers.pywhich validates:Verification
python -m unittest discover -s test).python -m unittest test/test_CLSM_bh_markers.py).