Skip to content

172 create duration spec and update fly and step #176

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 38 commits into from
Jul 31, 2025
Merged

172 create duration spec and update fly and step #176

merged 38 commits into from
Jul 31, 2025

Conversation

LuisFSegalla
Copy link
Contributor

This Pull Request aims at adding a new spec class Duration that contains only a Float. This class is now used in fly and step as a new Dimension that is zipped with the other ones.

@LuisFSegalla LuisFSegalla linked an issue Jul 10, 2025 that may be closed by this pull request
Create Duration spec and update fly and step #172
Closed
coretl
coretl requested changes Jul 10, 2025
View reviewed changes
@LuisFSegalla LuisFSegalla requested a review from coretl July 11, 2025 14:44
coretl
coretl requested changes Jul 14, 2025
View reviewed changes
Copy link
Contributor

@coretl coretl left a comment

Choose a reason for hiding this comment

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

Please can you also remove all references to DURATION and the Static.duration classmethod

@LuisFSegalla LuisFSegalla marked this pull request as draft July 18, 2025 12:54
@LuisFSegalla LuisFSegalla requested a review from coretl July 18, 2025 12:54
@LuisFSegalla
Copy link
Contributor Author

Changed this PR to be a draft for the time being as I'm not sure this is the right way to implement what was discussed with @coretl.

coretl
coretl requested changes Jul 22, 2025
View reviewed changes
@LuisFSegalla LuisFSegalla requested a review from coretl July 23, 2025 15:20
coretl
coretl reviewed Jul 23, 2025
View reviewed changes
Copy link
Contributor

@coretl coretl left a comment

Choose a reason for hiding this comment

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

Hmm, I'm still a little unsure about whether we've got the typing right, but if you do these bits then we can explore in a different ticket

@LuisFSegalla
Copy link
Contributor Author

LuisFSegalla commented Jul 24, 2025 •
edited
Loading

It was decided that in a Product only the inner spec can define the duration.
We'll try to default spec in ConstantDuration to None and move it to the second argument and see if typing will complain.
Change ConsntantDuration.calculate to create a new empty Dimension with only the duration if no spec was provided.
Change ConsntatDuration.calculate if fly == False to call self.spec.calculate(bounds=False) and set duration on the last Dimension, changing the lenght to be based on .gap not .midpoints

@LuisFSegalla LuisFSegalla requested a review from coretl July 25, 2025 07:33
coretl
coretl reviewed Jul 25, 2025
View reviewed changes
Copy link
Contributor

@coretl coretl left a comment

Choose a reason for hiding this comment

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

One more suggestion, then I think the code is done.

Please could you add unit tests for all the if branches in the new code that are not currently covered:
https://app.codecov.io/gh/bluesky/scanspec/pull/176?dropdown=coverage&src=pr&el=h1

coretl
coretl reviewed Jul 25, 2025
View reviewed changes
Copy link
Contributor

@coretl coretl left a comment

Choose a reason for hiding this comment

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

Found a few more...

@LuisFSegalla LuisFSegalla requested a review from coretl July 25, 2025 14:20
@LuisFSegalla LuisFSegalla marked this pull request as ready for review July 28, 2025 08:00
LuisFSegalla
LuisFSegalla commented Jul 28, 2025
View reviewed changes
LuisFSegalla
LuisFSegalla commented Jul 28, 2025
View reviewed changes
LuisFSegalla
LuisFSegalla commented Jul 28, 2025
View reviewed changes
LuisFSegalla
LuisFSegalla commented Jul 28, 2025
View reviewed changes
LuisFSegalla
LuisFSegalla commented Jul 28, 2025
View reviewed changes
LuisFSegalla
LuisFSegalla commented Jul 28, 2025
View reviewed changes
LuisFSegalla
LuisFSegalla commented Jul 28, 2025
View reviewed changes
LuisFSegalla
LuisFSegalla commented Jul 28, 2025
View reviewed changes
LuisFSegalla
LuisFSegalla commented Jul 28, 2025
View reviewed changes
LuisFSegalla
LuisFSegalla commented Jul 28, 2025
View reviewed changes
LuisFSegalla
LuisFSegalla commented Jul 28, 2025
View reviewed changes
LuisFSegalla
LuisFSegalla commented Jul 28, 2025
View reviewed changes
LuisFSegalla and others added 15 commits July 29, 2025 10:53
@LuisFSegalla
Enforced version of Pillow to fix broken cli tests
51635a6
@LuisFSegalla
Removing unreacheable part of the code
c04a517
@LuisFSegalla
Removing fly function
5c37b6f
@coretl @LuisFSegalla
Remove type ignores
b97a946
@LuisFSegalla
Moved Pillow dependency to dev
466bd52
@LuisFSegalla
Updated check statements on Dimension class
5e09873
@LuisFSegalla
Made bounds default to false and changed Duration return
8127d55 
Default value of the bounds argument is now `False`. Needed to change
the `frames` method in the `Spec` class so that it would call calculate
with the correct bounds, otherwise it wouls break some tests.

Made it so that the `duration` method in the base class `Spec` returned
None by default and removed the implementations from classes that would
always return None.
@LuisFSegalla
Updated tests based on new bounds change
633b197
@LuisFSegalla
Updated spec.py
b4e9470 
Added support for __rmatmul__

Added fly function back but put a deprecation warning on it and the step
function
@LuisFSegalla
Updated tests
1be894d 
tests_cli needed to be changed because previously, the default value for
bounds was `True` which would imply now that we're doing a fly scan.
This is not the default anymore.

Updated specs tests so that they would also catch the error messages
@LuisFSegalla
Updated test
63f86ea 
Added new tests for fly, step and get_constant_duration functions.

Also updated their warning messages
@LuisFSegalla
Updated docs
27e4038
@LuisFSegalla
Updated README.md
0ecbfe4
@LuisFSegalla
Fix typos
9c81919
@LuisFSegalla
Remove DURATION after rebase
0322a09
@LuisFSegalla LuisFSegalla force-pushed the 172-create-duration-spec-and-update-fly-and-step branch from 59a8d92 to 0322a09 Compare July 29, 2025 11:45
@LuisFSegalla
Small changes
ec79cb0 
Reverting test_rect_region_difference to a fly scan
Added return values to duration method on Fly class
@LuisFSegalla
Fixing __rmatmul__
698016d
@LuisFSegalla
Updated tests to improve code coverage
8480790
@shihab-dls
Copy link
Contributor

"Anywhere in the tests where gap output has changed, can we put it back, and change the spec so the gap output is the same?"

coretl
coretl reviewed Jul 30, 2025
View reviewed changes
Copy link
Contributor

@coretl coretl left a comment

Choose a reason for hiding this comment

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

Almost there...

@LuisFSegalla LuisFSegalla mentioned this pull request Jul 31, 2025
Merged
@LuisFSegalla LuisFSegalla requested a review from coretl July 31, 2025 08:55
@LuisFSegalla LuisFSegalla merged commit 117d295 into main Jul 31, 2025
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@coretl coretl coretl approved these changes

@shihab-dls shihab-dls Awaiting requested review from shihab-dls

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Create Duration spec and update fly and step
3 participants
@LuisFSegalla @shihab-dls @coretl