Skip to content

[essimaging] Publish reduced scitiff from Odin bragg edge workflow#641

Merged
nvaytet merged 11 commits into
mainfrom
publish-reduce-scitiff
Jun 24, 2026
Merged

[essimaging] Publish reduced scitiff from Odin bragg edge workflow#641
nvaytet merged 11 commits into
mainfrom
publish-reduce-scitiff

Conversation

@nvaytet

@nvaytet nvaytet commented Jun 17, 2026

Copy link
Copy Markdown
Member

We add a unit test that publishes a normalized image as a scitiff file.
This is to be picked up by analysis team for further tests.

The would like to have tof as the 3rd dimension, so we add a provider to compute it.

@nvaytet nvaytet added the essimaging Issues for essimaging. label Jun 17, 2026
@github-actions github-actions Bot added essnmx Issues for essnmx. essreduce Issues for essreduce. labels Jun 17, 2026
@nvaytet

nvaytet commented Jun 17, 2026

Copy link
Copy Markdown
Member Author

Needs scipp/scitiff#56

@nvaytet nvaytet marked this pull request as ready for review June 24, 2026 13:00
@nvaytet nvaytet requested a review from YooSunYoung June 24, 2026 13:13
Comment on lines +103 to +105
x=normed.coords['x_pixel_offset'],
y=normed.coords['y_pixel_offset'],
t=sc.midpoints(normed.coords['tof']),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we should keep the coordinate names as they are, no...?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I just assumed that people reading the images expect to have x/y as coordinates instead of our strange 'pixel_offset' coords?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Also, we rename the dims below, so it sort of makes sense that the coords have the same name as the dims? But maybe I misunderstood the Scitiff data structure?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The dimension names are limited so that we make sure of the order of those dimensions.
It's because, imaging processing SW such as ImageJ often assumes certain order of dimensions.

Coordinate names should stay informative so that people know what they actually mean on the other hand.
However, it doesn't matter too much since they will have units.... I guess...?

Like, t dimension can have either tof or wavelength but unit will tell you which is which.
It'll be more problematic if they have same units like tof and event_time_offset, but this is less likely to happen...?

About the x/y_pixel_offset. I think it is important to keep them as they are. It'll be less confusing even.
For example, x_pixel_offset doesn't represent its actual x-position of pixel in a certain coordinate.
x_pixel_offset is clear that it's only referring to the detector plane.

@YooSunYoung YooSunYoung left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good to me except for the minor comment about the coordinate names in the output file.

@nvaytet nvaytet added this pull request to the merge queue Jun 24, 2026
Merged via the queue into main with commit 790a391 Jun 24, 2026
24 checks passed
@nvaytet nvaytet deleted the publish-reduce-scitiff branch June 24, 2026 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

essimaging Issues for essimaging. essnmx Issues for essnmx. essreduce Issues for essreduce.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants