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

Update shutter behavior and LF acquisition channel order #160

Merged
merged 21 commits into from
Sep 12, 2022

Conversation

ieivanov
Copy link
Contributor

@ieivanov ieivanov commented Aug 29, 2022

This PR fixes #111. It also starts to address #110 and #142

To fully fix #142 we also need to:

  • ask the user to manually close and open the shutter if an auto shutter is not available
  • provide the option to manually enter a black level - either in the GUI or the CLI
    @talonchandler could you please help me with those?

Towards addressing #110 I've added a check to ensure that all LF channel exposures are the same. If they are not, all exposures are set to the exposure for State0. That's not a complete solution, but will prevent acquiring corrupted data.

@ieivanov ieivanov added this to the 0.2.0 milestone Aug 29, 2022
@codecov-commenter
Copy link

codecov-commenter commented Aug 29, 2022

Codecov Report

Merging #160 (ef9de8f) into main (3c570ec) will increase coverage by 0.09%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main     #160      +/-   ##
==========================================
+ Coverage   11.20%   11.30%   +0.09%     
==========================================
  Files          45       45              
  Lines        5990     5938      -52     
==========================================
  Hits          671      671              
+ Misses       5319     5267      -52     
Impacted Files Coverage Δ
recOrder/acq/acq_functions.py 0.00% <0.00%> (ø)
recOrder/calib/Calibration.py 0.00% <0.00%> (ø)
recOrder/plugin/widget/main_widget.py 0.00% <0.00%> (ø)
recOrder/plugin/workers/acquisition_workers.py 0.00% <0.00%> (ø)
recOrder/plugin/workers/calibration_workers.py 0.00% <0.00%> (ø)
recOrder/io/utils.py 0.00% <0.00%> (ø)
recOrder/pipelines/qlipp_pipeline.py 0.00% <0.00%> (ø)

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

@deprecated-napari-hub-preview-bot
Copy link

deprecated-napari-hub-preview-bot bot commented Aug 29, 2022

Preview page for your plugin is ready here:
https://preview.napari-hub.org/mehta-lab/recOrder/160
Updated: 2022-09-12T21:30:29.088358

@talonchandler
Copy link
Collaborator

talonchandler commented Aug 29, 2022

This is excellent, Ivan, thanks so much. I'll definitely be able to take this over the finish line.

I really like your "warning + default to State0" solution for #110---good call. This fix moved to #162.

In the interests of onboarding Ziwen, I'm thinking of testing and completing the fixes for #110, #111, and #142 on three different branches. Thanks for giving us a major headstart!

@talonchandler
Copy link
Collaborator

I've moved the exposure check to a different PR #162 since the implementation needs work, and this PR will benefit from narrowed scope.

No references to this code
All functionality was moved to `calibration_workers.py` and/or `main_widget.py`
@talonchandler talonchandler self-assigned this Sep 9, 2022
ziw-liu added a commit that referenced this pull request Sep 9, 2022
* Exposure check moved from #160

* seperate exposure check and acquisition methods

* simplify code checking and setting exposure times

* show the exposure time warning in napari

* Tighten warning message.

* stop forcing equal exposures and throw an error

* update docstring

* Minor update to error message.

Co-authored-by: Ziwen Liu <67518483+ziw-liu@users.noreply.github.com>
@talonchandler talonchandler marked this pull request as ready for review September 9, 2022 23:44
@talonchandler
Copy link
Collaborator

This PR is on the larger side. It includes:

Features

  • keeping the shutter open during multi-channel and multi-slice acquisitions
  • options for users to calibrate when a shutter isn't available
    • if a shutter isn't available, a warning appears prompting an input on the command line
    • the user can enter a number that will be used as a black level, or close the shutter manually and let recOrder estimate the black level
  • slice-first acquisition instead of channel-first acquisition

Non-features

  • To allow for testing of import statements, I have made napari[all] a requirement (and updated the docs accordingly).
  • Dropped support for python 3.7 because napari has also dropped support.

@ziw-liu much of this depends on hardware, but I'd still appreciate a quick second set of eyes on the code before merging. Thanks!

tox.ini Outdated Show resolved Hide resolved
@ziw-liu ziw-liu added the enhancement New feature or request label Sep 12, 2022
@talonchandler talonchandler merged commit a65c430 into main Sep 12, 2022
@talonchandler talonchandler deleted the bugfix-shutter-channelOrder branch September 12, 2022 22:06
@ieivanov
Copy link
Contributor Author

Quick note: I see that you've removed run_calibration, run_5state_calibration, and run_4state_calibration from calib.Calibration. These function used to let users run LC calibration outside of the recOrder widget (e.g. in a script). I think that's OK, as this is not a commonly used mode of recOrder right now, and we don't test and debug it.

@talonchandler
Copy link
Collaborator

Thanks for the context @ieivanov.

I'm totally in favor of script-based calibration if it's useful. For the sake of maintenance/updates, it will be great to have a single function that the GUI/scripts/others can call. I think we can revisit these design choices when we address #120.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
4 participants