Skip to content
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

Center-of-mass #94

Open
1 task
Tom-Willemsen opened this issue Dec 4, 2024 · 4 comments
Open
1 task

Center-of-mass #94

Tom-Willemsen opened this issue Dec 4, 2024 · 4 comments
Assignees

Comments

@Tom-Willemsen
Copy link
Contributor

Tom-Willemsen commented Dec 4, 2024

When doing a continuous scan using the laser/laser diode against sample changer pos on LOQ, we used bluesky's PeakStats callback to get a centre of mass. It returned an implausible result, well outside the actual peak.

i.e. for a peak that looked like this:

Image

(scan uid bed72c78-61bc-49c6-98ac-8974162b2e08)

we got a centre of mass of 507.4936482348703 - which just visually is clearly badly wrong.

A few things it could have been getting confused by:

  • The scan would not have had constant point spacing, was continuous motion so some motor acceleration/deceleration will be present (i.e. points close to edges will be closer together)
  • Some of the intensities were negative (range was about -7.5 .. +9)
  • The scan was a "there-and-back" scan so x points would be like: -2 -1 0 1 2 1 0 -1 -2

CoM should be able to cope with all of these, but suspect one of the above is not handled well and that's what caused a bad result.

Acceptance criteria

  • Investigate, fix upstream if possible, or if upstream won't accept a fix for this then write our own center-of-mass callback.

Planning

10/01/25 - 00:21:30
Discussed 19/12/20224 - agreed to push to next spring

@Tom-Willemsen
Copy link
Contributor Author

Proposed as CoM is a pretty "core" thing that lots of beamlines will need & it came up in LOQ bluesky testing.

@rerpha rerpha mentioned this issue Dec 16, 2024
5 tasks
@ISISBuilder ISISBuilder moved this to Backlog in PI_2024_08 Jan 10, 2025
@GRyall GRyall added the 3 label Jan 10, 2025
@KathrynBaker KathrynBaker moved this to Backlog in PI_2025_02 Jan 10, 2025
@jackbdoughty jackbdoughty self-assigned this Jan 14, 2025
@jackbdoughty jackbdoughty moved this from Backlog to In Progress in PI_2025_02 Jan 14, 2025
@jackbdoughty
Copy link
Contributor

Made a PR on bluesky upstream. Now we wait.. bluesky/bluesky#1878

@jackbdoughty jackbdoughty moved this from In Progress to Impeded in PI_2025_02 Jan 30, 2025
@Tom-Willemsen
Copy link
Contributor Author

Tom-Willemsen commented Feb 1, 2025

In addition to the PR above, looks like bluesky's center_of_mass implementation also has an event-ordering dependency (probably due to use of np.interp which wants monotonic data, if I had to guess). This is likewise also contributing to the bad result we saw as we had a there-and-back scan.

e.g.

>>> ps1 = PeakStats("x", "y")
>>> ps1.start({})
>>> ps1.event({"data": {"x": 0, "y": 0}})
>>> ps1.event({"data": {"x": 0, "y": 0}})
>>> ps1.event({"data": {"x": 1, "y": 1}})
>>> ps1.event({"data": {"x": 1, "y": 1}})
>>> ps1.event({"data": {"x": 2, "y": 0}})
>>> ps1.event({"data": {"x": 2, "y": 0}})
>>> ps1.stop({})
>>> print(f'com 1: {ps1["com"]}')
com 1: 1.0

>>> ps2 = PeakStats("x", "y")
>>> ps2.start({})
>>> ps2.event({"data": {"x": 0, "y": 0}})
>>> ps2.event({"data": {"x": 1, "y": 1}})
>>> ps2.event({"data": {"x": 2, "y": 0}})
>>> ps2.event({"data": {"x": 2, "y": 0}})
>>> ps2.event({"data": {"x": 1, "y": 1}})
>>> ps2.event({"data": {"x": 0, "y": 0}})
>>> ps2.stop({})
>>> print(f'com 2: {ps2["com"]}')
com 2: 2.0

The events above are identical, only the order of events has changed - but the CoM is reported incorrectly in the second case.

This means that bluesky's CoM is currently not valid to use for things like adaptive scans which can backstep, or any other non-monotonic scan. This at least needs highlighting in our docs (and probably bluesky's upstream docs too). And then we need to either push another fix upstream or just write our own callback that handles this.

Concrete example with non-overlapping data that could be generated by a step scan with step=1 vs an adaptive scan with initial_step = 2 and min_step = 1 that measures exactly the same x/y points (and only needs 1 "swap" to demonstrate the bug):

>>> ps1 = PeakStats("x", "y")
>>> ps1.start({})
>>> ps1.event({"data": {"x": 0, "y": 0}})
>>> ps1.event({"data": {"x": 1, "y": 1}})
>>> ps1.event({"data": {"x": 2, "y": 2}})
>>> ps1.event({"data": {"x": 3, "y": 1}})
>>> ps1.event({"data": {"x": 4, "y": 0}})
>>> ps1.stop({})
>>> print(f'com 1: {ps1["com"]}')
2.0

>>> ps2 = PeakStats("x", "y")
>>> ps2.start({})
>>> ps2.event({"data": {"x": 0, "y": 0}})
>>> ps2.event({"data": {"x": 2, "y": 2}})
>>> ps2.event({"data": {"x": 1, "y": 1}})
>>> ps2.event({"data": {"x": 3, "y": 1}})
>>> ps2.event({"data": {"x": 4, "y": 0}})
>>> ps2.stop({})
>>> print(f'com 2: {ps2["com"]}')
1.25

@Tom-Willemsen
Copy link
Contributor Author

Tom-Willemsen commented Feb 1, 2025

Initial test suggests non-constant x point spacing might also be yet another issue - when I first skim-read the bluesky implementation it looked like they had used the resample & linear interpolate approach, but having tested it I don't think that's true.

At this point, probably best to write a completely new CoM callback, rather than trying to incrementally fix all these things in the existing one in PeakStats. I think initially implement it here, then in future upstream might also be more receptive to us adding a new/alternative callback, or perhaps a new key in PeakStats, rather than modifying the existing PeakStats["com"] implementation - which other facilities might depend on the behaviour of, for better or worse, despite the things listed above.

@jackbdoughty jackbdoughty moved this from Impeded to In Progress in PI_2025_02 Feb 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In Progress
Status: Backlog
Development

No branches or pull requests

5 participants