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

API evolution and performance improvements #74

Merged
merged 68 commits into from Apr 19, 2023
Merged

API evolution and performance improvements #74

merged 68 commits into from Apr 19, 2023

Conversation

sk1p
Copy link
Member

@sk1p sk1p commented Mar 15, 2023 •
edited

Update DECTRIS acquisition to pass data via shared memory to workers; this circumvents costly serialization for moving data between processes and makes it feasible to perform live reconstruction on less powerful systems (i.e. more processing power left over for actual processing versus overheads)

Also updates the APIs of LiveContext; adds a "passive mode", i.e. waiting for an external party to arm/trigger the detector.

Requires latest libertem_dectris release (0.2.4+).

TODO

  • Documentation updates
    • Update Merlin documentation to new API
    • Finish new DECTRIS docs
    • Update the general section of the docs
    • Include a dectris sample notebook in the docs
  • Add a changelog entry
  • Update example notebooks
  • Review example notebooks
  • API review
    • Think about the "generic" use case - one piece of software that uses our API to connect to whatever different detector
    • Right now, both the make_connection and the make_acquisition methods require customization per-detector type - this is sub-optimal, as you would need to have two "adapters" that feed the connection and the acquisition builder.
    • Maybe we can get away with reducing the acquisition constructor parameters to a common interface?
      • Example: merlin requires sig_shape currently - instead, we can GET the IMAGEX, IMAGEY parameters if we integrate a bit closer with the control part of the interface, in the initialize method.
      • Other parameters can be moved from the acquisition constructor to the connection, as they hopefully stay the same over time
      • Is there anything that would contradict a generic interface?
      • As an exercise, try to implement the aforementioned generic interface for all detector types we have in the pipeline (dectris/merlin plus k2is/x-spectrum/asi-tpx3)
  • Implement passive mode for merlin/memory/etc. (maybe separate PR)
    • Generally implement new API for Merlin/Memory acquisition (conn/aq split)
    • Add support for DetectorConnection.wait_for_acquisition
      • Merlin
      • Memory
  • See what issues from Testing at KIT: Wash list #63 can be addressed directly, and which may already be fixed
    • Some fixed parts checked off that list, some still remaining

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 sk1p mentioned this pull request Mar 20, 2023
Closed
72 tasks
@sk1p
Copy link
Member Author

sk1p commented Mar 21, 2023

/azp run libertem.libertem-live-data

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@sk1p
Copy link
Member Author

sk1p commented Mar 21, 2023

/azp run libertem.libertem-live-data

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@sk1p sk1p mentioned this pull request Mar 29, 2023
Open
9 tasks
@sk1p
Copy link
Member Author

sk1p commented Mar 30, 2023

/azp run libertem.libertem-live-data

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@sk1p
Copy link
Member Author

sk1p commented Mar 30, 2023

/azp run libertem.libertem-live-data

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@sk1p
Copy link
Member Author

sk1p commented Mar 31, 2023

/azp run libertem.libertem-live-data

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@sk1p
Copy link
Member Author

sk1p commented Apr 4, 2023

/azp run libertem.libertem-live-data

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@sk1p
Copy link
Member Author

sk1p commented Apr 4, 2023

/azp run libertem.libertem-live-data

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@sk1p
Copy link
Member Author

sk1p commented Apr 4, 2023

/azp run libertem.libertem-live-data

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@sk1p
Copy link
Member Author

sk1p commented Apr 5, 2023

/azp run libertem.libertem-live-data

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@sk1p
Copy link
Member Author

sk1p commented Apr 5, 2023

/azp run libertem.libertem-live-data

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@sk1p
Copy link
Member Author

sk1p commented Apr 5, 2023

/azp run libertem.libertem-live-data

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@sk1p sk1p added this to the 0.2 milestone Apr 5, 2023
@sk1p sk1p changed the title WIP: DECTRIS: shm-based data handling WIP: API evolution and performance improvements Apr 6, 2023
@sk1p
Copy link
Member Author

sk1p commented Apr 6, 2023

/azp run libertem.libertem-live-data

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@sk1p
Copy link
Member Author

sk1p commented Apr 6, 2023

/azp run libertem.libertem-live-data

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@sk1p
Update dectris example notebook
9ba5b28 
Simulator was changed to use logging, so we can configure when we
actually want it to spew info on us. Also include `libertem_dectris`
logging environment variable setup.
@sk1p
Copy link
Member Author

sk1p commented Apr 11, 2023

/azp run libertem.libertem-live-data

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@sk1p
Copy link
Member Author

sk1p commented Apr 11, 2023

/azp run libertem.libertem-live-data

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@codecov
Copy link

codecov bot commented Apr 11, 2023 •
edited

Codecov Report

Patch coverage: 83.92% and project coverage change: +51.97 🎉

Comparison is base (a0281f7) 25.15% compared to head (136898c) 77.13%.

Additional details and impacted files 
@@             Coverage Diff             @@
##           master      #74       +/-   ##
===========================================
+ Coverage   25.15%   77.13%   +51.97%     
===========================================
  Files          23       31        +8     
  Lines        2457     2943      +486     
  Branches      321      410       +89     
===========================================
+ Hits          618     2270     +1652     
+ Misses       1836      544     -1292     
- Partials        3      129      +126
Impacted Files Coverage Δ
src/libertem_live/detectors/dectris/sim.py 58.55% <40.00%> (+58.55%) ⬆️
src/libertem_live/detectors/dectris/controller.py 65.75% <65.75%> (ø)
src/libertem_live/api.py 80.00% <69.56%> (+50.27%) ⬆️
src/libertem_live/detectors/base/acquisition.py 78.57% <76.31%> (+7.14%) ⬆️
src/libertem_live/detectors/merlin/data.py 83.09% <83.92%> (+62.70%) ⬆️
src/libertem_live/detectors/dectris/acquisition.py 85.00% <86.91%> (+57.78%) ⬆️
src/libertem_live/hooks.py 87.50% <87.50%> (ø)
src/libertem_live/detectors/dectris/connection.py 88.29% <88.29%> (ø)
src/libertem_live/detectors/merlin/sim.py 78.06% <90.00%> (+58.93%) ⬆️
src/libertem_live/detectors/merlin/acquisition.py 92.89% <92.00%> (+60.15%) ⬆️
... and 12 more

... and 3 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@sk1p
Copy link
Member Author

sk1p commented Apr 11, 2023

/azp run libertem.libertem-live-data

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@sk1p sk1p changed the title WIP: API evolution and performance improvements API evolution and performance improvements Apr 11, 2023
@sk1p
Copy link
Member Author

sk1p commented Apr 11, 2023

@uellue @matbryan52 this PR is now ready from my side. I realize it is once again more of a feature branch than a single isolated change, but such is the nature if the whole API is revamped...

This should now be more or less stable, and the API changes are done as discussed. I did not yet put more work into getting the merlin part back into feature parity with the DECTRIS part. This is basically #75, and can be done once this is merged, or even after the release. We should probably reserve some time at the HF5000 for this, to check real system behavior.

I'll work next to get the ASI TPX3 stuff into PR form, so we can include it at least as experimental support.

@sk1p
Copy link
Member Author

sk1p commented Apr 11, 2023

/azp run libertem.libertem-live-data

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@sk1p sk1p mentioned this pull request Apr 11, 2023
Closed
5 tasks
@matbryan52
Copy link
Member

@uellue @matbryan52 this PR is now ready from my side. I realize it is once again more of a feature branch than a single isolated change, but such is the nature if the whole API is revamped...

Great! I've been using the branch for the UI work in the last week, so far good but I still need to integrate the passive / wait_for_acquisition mode. I will try to do that today and then look into the PR in more detail.

@sk1p sk1p mentioned this pull request Apr 17, 2023
Merged
24 tasks
uellue
uellue requested changes Apr 18, 2023
View reviewed changes
Copy link
Member

@uellue uellue left a comment

Choose a reason for hiding this comment

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

Very nice! Thanks a lot for all this. A number of small comments and suggestions in separate comments. 😄

docs/source/usage.rst Outdated Show resolved Hide resolved
docs/source/usage.rst Outdated Show resolved Hide resolved
docs/source/usage.rst Outdated Show resolved Hide resolved
docs/source/usage.rst Outdated Show resolved Hide resolved
docs/source/usage.rst Outdated Show resolved Hide resolved
docs/source/detectors/dectris.rst Outdated Show resolved Hide resolved
docs/source/detectors/dectris.rst Show resolved Hide resolved
docs/source/detectors.rst Show resolved Hide resolved
src/libertem_live/detectors/merlin/acquisition.py Outdated Show resolved Hide resolved
docs/source/usage.rst Show resolved Hide resolved
sk1p and others added 2 commits April 18, 2023 14:06
@sk1p @uellue
Apply suggestions from code review
bb6853e 
Co-authored-by: Dieter Weber <d.weber@materials-scientist.com>
@sk1p @uellue
Documentation updates from review
136898c 
Co-authored-by: Dieter Weber <d.weber@fz-juelich.de>
@sk1p
Copy link
Member Author

sk1p commented Apr 18, 2023

/azp run libertem.libertem-live-data

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@sk1p sk1p requested a review from uellue April 18, 2023 14:06
@sk1p sk1p mentioned this pull request Apr 18, 2023
Merged
@uellue
Copy link
Member

uellue commented Apr 19, 2023

LGTM, great! 👍

@uellue uellue merged commit 3143ed3 into LiberTEM:master Apr 19, 2023
16 checks passed
@sk1p sk1p deleted the dectris-shm branch April 19, 2023 08:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@uellue uellue Awaiting requested review from uellue

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

Successfully merging this pull request may close these issues.

None yet
3 participants
@sk1p @matbryan52 @uellue