Skip to content

Bugfix linearsource scanspecv2#213

Open
LuisFSegalla wants to merge 4 commits into
v2-devfrom
bugfix-linearsource-scanspecv2
Open

Bugfix linearsource scanspecv2#213
LuisFSegalla wants to merge 4 commits into
v2-devfrom
bugfix-linearsource-scanspecv2

Conversation

@LuisFSegalla

@LuisFSegalla LuisFSegalla commented May 15, 2026

Copy link
Copy Markdown
Contributor

While testing ScanSpecV2 I noticed that if trying to create an Acquire spec with fly=true and then tried making this into a scan and parsing the windows the positional values returned by the windows would be out of the expected boundaries of my motion spec. I believe this is to do with how the LinearSource is defined where we have it use the original length passed to it to define the step size, independent of the size of the index passed in setpoints.
This PR tries to address this by changing the way the step size is calculated and by adding some Unit tests to it.

Here's an example of the code I used to tests and find the initial problem:

from scanspec2.core import DetectorGroup
from scanspec2.specs import Acquire, Linspace, Static

det1 = DetectorGroup(1, 10, 1, 1, ["spec_det1"])
det2 = DetectorGroup(1, 1, 1, 1, ["spec_det2"])

spec = Acquire(
    spec=Linspace("x", 0, 10, 10), fly=True, detectors=[det1], stream_name="spec"
)
scan = spec.compile()

for window in scan:
    for axis in window.moving_axes:
        print(f"axis: {window.moving_axes}")
    for trig in window.trigger_groups:
        print(f"trig.detectors = {trig.detectors}")
        for pattern in trig.trigger_patterns:
            print(
                f"livetime = {pattern.livetime}\ndeadtime = {pattern.deadtime}\nrepeats = {pattern.repeats}\n"
            )
    for p in window.positions(1, None):
        print(f"pos = {p['x']}\nlen(pos) = {len(p['x'])}")

Changed logic of LinearSource class to account for indexes.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm sure we've got this helper somewhere else in tests/scanspec2 already...

Comment thread tests/scanspec2/test_compile.py Outdated
# Detector with total acquisition time of 1s
det = DetectorGroup(1, 10, 0.9, 0.1, ["spec_det1"])
spec: Acquire[str, str, Never] = Acquire(
spec=Linspace("x", 0, 10, 10), fly=True, detectors=[det], stream_name="spec"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The numbers make my head hurt. Please could you redo the test with Linspace("x", 11, 20, 10)

Comment on lines +1030 to +1045
for p in w.positions(1, None):
assert len(p["x"]) == 10
assert p["x"] == approx(
[
-0.55555556,
0.67901235,
1.91358025,
3.14814815,
4.38271605,
5.61728395,
6.85185185,
8.08641975,
9.32098765,
10.55555556,
]
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Then I would expect this to have size 11, and be [0.5, ... 10.5] as we are returning bounds rather than midpoints

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't understand why this would be the expected behaviour. Currently all the method does is shift the first point to be the equivalent to the first lower bound and then move all the subsequent points based on that, but it does not increase the expected number of points to also include the last upper bound.
That should be an easy enough change to make though if that's the intended behaviour.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is the intended behaviour. The Spec should say "I want 10 points, each is 1s long". The positions function is asked "Give me points every 1s. That means it gets 11 points, from lower bound of the first point to upper bound of the last point. If it doesn't do this then we should modify it so it does.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Okay, I'll push the changes that implement this logic.

* Changed setpoints method in the LinearSource class so that both the
first lower bound and the last upper bound are returned.
@LuisFSegalla LuisFSegalla requested a review from coretl May 21, 2026 13:37
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.

2 participants