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

Add skipping output #180

Merged
merged 8 commits into from
Aug 30, 2023
Merged

Add skipping output #180

merged 8 commits into from
Aug 30, 2023

Conversation

abbiemery
Copy link
Collaborator

This is an option for propagation of skipped devices. It is not particularly elegant, but it does avoid changing the event routing or anything vital for now until we find a better option that we are happy with and that is optimal.

Copy link
Contributor

@callumforrester callumforrester left a comment

Choose a reason for hiding this comment

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

While we understand how this works, shall we write some docs?
Also could you make an issue to write tests for Ticker?

@abbiemery
Copy link
Collaborator Author

Don't worry, tests are coming. Will make a note in the dev docs though. There is a doc for how updates aree scheduled but i'll update it.

@codecov
Copy link

codecov bot commented Aug 29, 2023

Codecov Report

Patch coverage: 89.47% and project coverage change: +0.03% 🎉

Comparison is base (04a0c47) 94.65% compared to head (dbda9ab) 94.68%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #180      +/-   ##
==========================================
+ Coverage   94.65%   94.68%   +0.03%     
==========================================
  Files          41       41              
  Lines        1309     1318       +9     
==========================================
+ Hits         1239     1248       +9     
  Misses         70       70              
Files Changed Coverage Δ
src/tickit/core/management/schedulers/base.py 94.44% <77.77%> (-3.52%) ⬇️
src/tickit/core/management/ticker.py 100.00% <100.00%> (+3.84%) ⬆️
src/tickit/core/typedefs.py 97.77% <100.00%> (+0.27%) ⬆️

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

@abbiemery
Copy link
Collaborator Author

@callumforrester , @DiamondJoseph as requested, the issue to add further testing #183 .


Args:
message (Union[Interrupt, Output, ComponentException]): An Interrupt,
message (Union[Interrupt, Output, ComponentException, Skip]): An Interrupt,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should there be some nice name for this union of useful message types?

Copy link
Contributor

Choose a reason for hiding this comment

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

There is, it's called ComponentOutput. Also, does anything break if you just remove the type from the docstring if it's in the function signature? Seems pointless to duplicate it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What was there initially should instead be the defined type alias ComponentOutput. However, I intentionally left skip out of it because it doesn't nicely fit in the Input or Output message types, since they are not sent like the other messages. I felt it prudent to keep the differentiation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@callumforrester No, nothing breaks per say, its just the Google doc string convention which we have kept to for the rest of the code base, so I'm reluctant to change it in one place.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What should have been flagged, is the function signature has the wrong type in, so I'll fix that now.

Copy link
Contributor

@DiamondJoseph DiamondJoseph left a comment

Choose a reason for hiding this comment

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

It worked for my use case, so if Callum is happy with it my comments are only nits (left as comments to not block merging)

Copy link
Contributor

@callumforrester callumforrester left a comment

Choose a reason for hiding this comment

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

Made an issue for further tests: #184

@abbiemery
Copy link
Collaborator Author

@callumforrester , @DiamondJoseph as requested, the issue to add further testing #183 .

@callumforrester no need for #184 , I'll roll them together.

@abbiemery abbiemery merged commit eba276e into master Aug 30, 2023
@abbiemery abbiemery deleted the add_skipping_output branch August 30, 2023 12:52
@abbiemery abbiemery linked an issue Aug 30, 2023 that may be closed by this pull request
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.

Ticks not completing if there is no delta for a system simulation
3 participants