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

Recording rework #176

Draft
wants to merge 34 commits into
base: dev
Choose a base branch
from
Draft

Recording rework #176

wants to merge 34 commits into from

Conversation

jacopoabramo
Copy link
Collaborator

An attempt to rework the RecordingManager, also mentioned in #171:

  • added the napari threading infrastructure to the basic framework
  • changed the Storer base class from abc.ABC to SignalInterface
    • this gives the possibility to send Qt signals to the GUI to update on the current recording status
  • added stream method to HDF5Storer and TiffStorer
  • added BytesIOStorer class to keep the possibility to store data in RAM for reconstruction
    • this is still untested
  • full rework of the RecordingManager
    • for each detector, a StreamingWorker thread, which is a napari wrapper over the QRunnable class, is spawned
    • a storer object is created and associated to each thread
    • acquisition loops are called within the threads
    • NOTE: timelapse acquisition is currently NOT supported

- moved `SaveMode`
- moved `SaveFormat`

This is part of a refactoring work of the manager.
- possibility to return a list of frame IDs as an union
- now inherits from `SignalInterface`
- added method for unpacking data chunks to evaluate missing frames
- automatically made at startup
- now passing the `detectorManager` reference directly
- `imageProcessing`: dictionary for storing image processing algorithms
  - for metadata storage
  - note: this may change
- `dtype`: detector data type
  - default value: `uint16`
- `frameInterval` time difference between acquired frames
  - reference unit: microseconds
  - default value: 1000
- Using multiple workers, one for each detector
@jacopoabramo
Copy link
Collaborator Author

@jonatanalvelid could this rework be cause of problems in case one would use different detectors but maybe they're synchronized with some scanners?

@kasasxav I tried replicating as close as possible your original work so to maintain a resemblance of retro-compatibility but I could not understand how you handle the case where the SaveMode.RAM option is selected. You store the frames in RAM but at the same time you open some HDF5 files? At least that's what specified in the failing unit tests too.

@kasasxav
Copy link
Collaborator

kasasxav commented Jun 16, 2023

Hey,

Yes, so ImSwitch has three modules so far: imcontrol, imreconstruct, imscripting.
The imreconstruct is something we use in order to have image processing algorithms that turn the raw data into a reconstructed image, which is useful in for example parallelized resolft.
For that reason, whenever we save in RAM what it means is that the images are stored in RAM and passed to the imreconstruct module so that they can be reconstructed without saving a hdf5 file.

Another option is to both save in files and RAM, and the benefit is that you will still have the raw data stored in your disk but you don't need to load it again for reconstruction, you'll have it directly in the reconstruction module.

@jonatanalvelid
Copy link
Collaborator

@jacopoabramo As far as I can see (with my still limited overview of the RecordingManager I have to say), I don't think it will affect using APDs or other point detectors. What is implemented so far for point-scanning in terms of detectors are APDs. However, in the point-scanning the "recording" is simply handled after an image has been acquired by the microscope. The current image (nD) is always just stored in the APDManager where it is built up during the acquisition row-by-row, by the APDManager continuously pulling whatever data it can from the buffer in the NiDAQ connected to the counter input. I.e. the APDs are not synchronized to scanners, image building happens afterwards by knowing what the scan curves looked like.
This means that the RecordingManager will only do something when you press Save after an image has been acquired, and it will then save that nD image from the APDManager to disk. In that sense, for point-scanning imaging, the Record button and corresponding functions is never used, except when you want to do a timelapse recording from the RecordingWidget (sometimes can be used as it supports frame-to-frame wait times, which the current point-scan signal designer does not), but this you specify already is not currently working overall.
So as long as this functionality is conserved for the Save button calls it should not be a problem for the APDs.

@jacopoabramo
Copy link
Collaborator Author

jacopoabramo commented Jun 16, 2023

@kasasxav what I don't understand from the unit test then, is why the test tries to read back from disk these h5py file that according to what you said should not be generated at all. See here.

@jacopoabramo
Copy link
Collaborator Author

jacopoabramo commented Jun 19, 2023

So... I think that I managed to solve the issues with the failing unit tests. I have to make some more changes (especially in relation to the Zarr storage, which is just missing). My question though is: why the hell is it taking so much time to run the test_recording_spec_time? Basically I had to set the timeout to None to make the test pass...

@kasasxav was there a particular reason to add the timeout to the unit tests? Or it's not mandatory?

@kasasxav
Copy link
Collaborator

Hi Jacopo,

I don't remember sorry :( I would say you can remove it.

Amazing work btw <3

@kasasxav
Copy link
Collaborator

kasasxav commented Jun 21, 2023

Ah sorry, I checked but I didn't answer.

I agree with you, it seems that it's only saving in RAM because of the attribute and then it tries to read from the hdf5! Weird.
I would say you can change that too, either maybe changing so that the files are recorded and then read, or having only RAM.

It's strange becuase these tests were passing at some point so I don't understand how.

- the new `RecordingManager` takes a lot of time to respect unit tests.
- removing the timeout does the job
- not really a solution
@jacopoabramo
Copy link
Collaborator Author

jacopoabramo commented Jun 23, 2023

So the testing for Windows and macOS went in timeout. I'm thinking that this rework doesn't work well with the unit testing but I really don't know how to fix it. I'll keep the PR open, maybe somebody can give me some suggestions.

@jacopoabramo jacopoabramo marked this pull request as draft June 30, 2023 08:40
- inheriting from `QRunnable`;
- usable for one-shot concurrent tasks;
- executed in the `RunnablePool` threadpool.
- now inherits from `Runnable`;
- moved signals to separate class `StorerSignals`;
  - this is due to inheritance problems with the `QRunnable` class;
- moved to separate folder `storers`;
- implemented `HDF5StorerManager`;
- implemented `TIFFStorerManager`;
- implemented `ZarrStorerManager`;
  - `stream` still missing;
- storer classes moved to separate folder;
- added a `runnablePool` object for a local threadpool;
  - now threads are automatically dispatched from it;
- refactored the `snap` function
- reduced recording test time to 1 sec;
  - tests were failing due to high frame interval;
@jacopoabramo
Copy link
Collaborator Author

New attempt to make the tests pass, and learned a couple of things (at least at unit-test level):

  • spawning multiple Thread objects was causing only the last thread in the thread list to be executed, when multiple cameras are instantiated. This is quite curious and I cannot explain why this happens; after reviewing the PyQt documentation apparently the best use case for multiple running threads to execute rely on using the QRunnable class and the QThreadPool class. In this way workers are automatically assigned to a thread and these seem to be outside the Qt event loop. Not sure why, but with this approach the concurrent streaming object workflow works (at least locally on Windows)
  • fully the RecordingManager file; now all the managers have been moved to a separate folder for an easier time to read everything.
  • the BytesIOStreamer class has been removed.
  • tests have been adjusted in order to be adapted to the new code (mostly for the initialization part; previous test are slightly adjusted but mostly work the same way).

@jacopoabramo
Copy link
Collaborator Author

test_recording_spec_frames[detectorInfos1] fails randomically. Not sure why, but apparently the datasets are empty.

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.

None yet

3 participants