Skip to content

Added additional msgpack data types for rs.io.read_dials_stills#326

Open
LuisA92 wants to merge 6 commits intors-station:mainfrom
LuisA92:main
Open

Added additional msgpack data types for rs.io.read_dials_stills#326
LuisA92 wants to merge 6 commits intors-station:mainfrom
LuisA92:main

Conversation

@LuisA92
Copy link
Copy Markdown
Collaborator

@LuisA92 LuisA92 commented Dec 23, 2025

I encountered errors when usingrs.io.read_dials_stills(extra_cols=['bbox', 'entering']). The issue is resolved by adding additional data types for the problematic columns.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Dec 23, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.78%. Comparing base (3621f28) to head (89f776b).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #326   +/-   ##
=======================================
  Coverage   86.78%   86.78%           
=======================================
  Files          40       40           
  Lines        2837     2837           
=======================================
  Hits         2462     2462           
  Misses        375      375           
Flag Coverage Δ
unittests 86.78% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@kmdalton kmdalton requested a review from dermen December 23, 2025 13:26
@kmdalton
Copy link
Copy Markdown
Member

This looks innocuous to me, but I'd like to ask @dermen to have a look before merging. Thanks, @LuisA92

@JBGreisman
Copy link
Copy Markdown
Member

seems reasonable to me too. Once @dermen approves, looks like a good fix to improve coverage of different dials outputs

@LuisA92
Copy link
Copy Markdown
Collaborator Author

LuisA92 commented Dec 27, 2025

For reference, the following columns from an integrated.refl work with the additional data types:

extra_cols = [
'background.mean',
 'background.sum.value',
 'background.sum.variance',
 'bbox',
 'd',
 'entering',
 'flags',
 'id',
 'imageset_id',
 'intensity.prf.value',
 'intensity.prf.variance',
 'intensity.sum.value',
 'intensity.sum.variance',
 'lp',
 'miller_index',
 'num_pixels.background',
 'num_pixels.background_used',
 'num_pixels.foreground',
 'num_pixels.valid',
 'panel',
 'partial_id',
 'partiality',
 'profile.correlation',
 'qe',
 'refl_ids',
 's1',
 'xyzcal.mm',
 'xyzcal.px',
 'xyzobs.mm.value',
 'xyzobs.mm.variance',
 'xyzobs.px.value',
 'xyzobs.px.variance',
 'zeta',
 ]

@kmdalton
Copy link
Copy Markdown
Member

it's probably okay to merge this if we can't raise @dermen. it don't think it should break any of his workflows.

Copy link
Copy Markdown
Member

@kmdalton kmdalton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a sensible extension that enables new machine learning applications.

@kmdalton
Copy link
Copy Markdown
Member

kmdalton commented Feb 1, 2026

@LuisA92 -- I think you should feel free to merge this if you're confident in it. It looks good / safe to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants