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.

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

Update MIB simulator #159

Merged
merged 8 commits into from
Jul 30, 2024
Merged

Update MIB simulator #159

merged 8 commits into from
Jul 30, 2024

Conversation

sk1p
Copy link
Member

@sk1p sk1p commented Jul 23, 2024 •
edited
Loading

  • Allow multiple acquisitions per connection in triggered mode
  • Fix acquisition header and sequence numbers in continuous mode and enforce disabled cache in this case
  • More correct control socket handling - read and use the MPX prefix

Contributor Checklist:

  • I have added or updated my entry in the creators.json file
  • I have added a changelog entry for my contribution
  • I have added/updated documentation for all user-facing changes
  • I have added/updated test cases

Reviewer Checklist:

  • /azp run libertem.libertem-live-data passed

sk1p added 3 commits July 22, 2024 16:17
@sk1p
merlin sim: allow multiple acquisitions on the same connection
e85c9ab 
Now it behaves a bit more like the "real deal", where previously it
would just silently accept STARTACQUISITION commands but ignore them.

Also switch over `test_acquisition_triggered_control` to a non-draining
sim and connection, as draining should no longer be the common case.
We still have `test_acquisition_triggered_garbage` to have coverage for
the draining case.

Fix the control connection to properly parse the mpx prefix to make sure
we don't get a short read from the control socket.
@sk1p
Improve BrokenPipeError diagnostics
c960359
@sk1p
Fix for sequence numbers in continuous mode
bca1240 
Previously, sequence numbers would be sent as they are in the file, but
this is of course not correct - they need to continue monotonously.
Only works in uncached mode, so enforce that.

Also warn if the user specifies `max_runs` and not `continuous`.
@sk1p sk1p requested a review from matbryan52 July 23, 2024 09:51
@sk1p
Copy link
Member Author

sk1p commented Jul 23, 2024

/azp run libertem.libertem-live-data

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@codecov Codecov
Copy link

codecov bot commented Jul 23, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 64.86486% with 26 lines in your changes missing coverage. Please review.

Project coverage is 79.49%. Comparing base (e680b94) to head (e53090f).

Files Patch % Lines
src/libertem_live/detectors/merlin/sim.py 65.27% 21 Missing and 4 partials ⚠️
src/libertem_live/detectors/common.py 50.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master     #159      +/-   ##
==========================================
- Coverage   79.84%   79.49%   -0.36%     
==========================================
  Files          35       35              
  Lines        3458     3491      +33     
  Branches      576      582       +6     
==========================================
+ Hits         2761     2775      +14     
- Misses        560      577      +17     
- Partials      137      139       +2

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

@sk1p
Copy link
Member Author

sk1p commented Jul 23, 2024

/azp run libertem.libertem-live-data

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@sk1p
Copy link
Member Author

sk1p commented Jul 23, 2024

/azp run libertem.libertem-live-data

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@sk1p
Copy link
Member Author

sk1p commented Jul 23, 2024

/azp run libertem.libertem-live-data

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@sk1p sk1p mentioned this pull request Jul 23, 2024
Closed
4 tasks
@sk1p
Copy link
Member Author

sk1p commented Jul 23, 2024

/azp run libertem.libertem-live-data

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@sk1p sk1p mentioned this pull request Jul 24, 2024
Merged
9 tasks
@sk1p
ctx_pipelined: increase timeouts
7da860f 
Might increase the success chance when the CI machine is overwhelmed...
@sk1p
Copy link
Member Author

sk1p commented Jul 24, 2024

/azp run libertem.libertem-live-data

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

matbryan52
matbryan52 requested changes Jul 26, 2024
View reviewed changes
Copy link
Member

@matbryan52 matbryan52 left a comment

Choose a reason for hiding this comment

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

I don't know how one would write it but we're missing a test case in continuous mode. Is that something that will be easier to add once the dependent PRs are merged?

@sk1p @matbryan52
Update src/libertem_live/detectors/merlin/sim.py
e53090f 
Co-authored-by: Matthew Bryan <78845903+matbryan52@users.noreply.github.com>
@sk1p
Copy link
Member Author

sk1p commented Jul 29, 2024

Thanks for the review!

I don't know how one would write it but we're missing a test case in continuous mode. Is that something that will be easier to add once the dependent PRs are merged?

I also noticed that continuous mode is now one of the last remaining "blind spots" of coverage. And indeed, right now, the client part of continuous mode is only accessible using the low-level APIs, which will be replaced by the rust-based backend, so I'll add a test case in #161.

@sk1p sk1p requested a review from matbryan52 July 29, 2024 07:25
@sk1p
Copy link
Member Author

sk1p commented Jul 29, 2024

/azp run libertem.libertem-live-data

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@sk1p
Copy link
Member Author

sk1p commented Jul 29, 2024

/azp run libertem.libertem-live-data

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@sk1p sk1p merged commit 8b532df into LiberTEM:master Jul 30, 2024
30 of 31 checks passed
@sk1p sk1p deleted the mib-sim-multi-acq branch July 30, 2024 07:22
@sk1p sk1p added this to the 0.3 milestone Oct 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matbryan52 matbryan52 matbryan52 approved these changes

Assignees
No one assigned
Labels
detector:merlin
Projects
None yet
Milestone
0.3
Development

Successfully merging this pull request may close these issues.
2 participants
@sk1p @matbryan52