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

Signal generators refactor and documentation #175

Open
jacopoabramo opened this issue Jun 14, 2023 · 11 comments
Open

Signal generators refactor and documentation #175

jacopoabramo opened this issue Jun 14, 2023 · 11 comments

Comments

@jacopoabramo
Copy link
Collaborator

jacopoabramo commented Jun 14, 2023

Today @jonatanalvelid kindly helped me getting more familiar with the current status of the signal generators within ImSwitch. @kasasxav, I apologize for tormenting you with this but I really need to make some order in my mind about this :P so whenever you have time to review this information it would be great.

First some terminology because this got me confused a lot, so it's just a reminder for me:

  • signal generator: the device manager that controls the hardware intended to generate signals which are then propagated with a specific pattern
  • signal designer: the controller that manages the creation of this pattern through some parameters usually defined at a config file level

There are currently some problems and questions which should be addressed:

  • there are some references within the APDManager to the NidaqManager class (for example here) which should not be there. The reason being that the signal generator in the APDManager could also use an interface different from the NidaqManager. Speaking with Jonatan we've come to realize that there should be some way to address this problem, but he's also currently working on it. This is just a reminder to keep this under watch and fix this whenever somebody manages to work on this;
  • in the SuperScanManager class there's a reference to this _parameterCompatibility method which is both abstract and implemented; is this a typo?
  • in the makeFullScan method, there's a reference to the scanInfoDict dictionary, which Jonatan told me is only used within the APDManager and there are some calculations being done to obtain this dictionary. Is there any reason why this should not be moved to the APDManager and add some parameters in the APD managerProperties if necessary?
  • the method runScanAdvanced, in the ScanController classes should probably be refactored to just a simple runScan, while makeFullScan in the ScanManager classes should be come something like buildScanSignal, unless I'm missing something;
  • in all the ScanController classes there's a direct reference to the nidaqManager instance which should not be there, but instead there should be a reference to a ScanManager base class. Funnily enough, @kasasxav, if you remember this is what we talked about a long time ago whenever I did my PulseStreamerManager implementation :D

EDIT: the issue mentiones documentation because there should be a proper vocabulary about this with some more detailed terminology in order to avoid confusion. Also, once we are sure about the points mentioned above, a proper tutorial on how to implement new signal generators and designers should be added in the documentation.

@jonatanalvelid
Copy link
Collaborator

jonatanalvelid commented Jun 15, 2023

Just a few comments from my side, to clarify some things:

  • The signal designers use, most importantly, also parameters from the ScanWidget.
  • The NidaqManager references in the APDManager is not "a problem" by itself in the sense that "they should not be there". It just happens to be that ImSwitch was originally built only supporting NiDAQ devices, not any general DAQ, and for that those references are perfectly fine to me.
  • However, I agree that to support other DAQs the NidaqManager and its references throughout the code should be replaced by a general DaqManager, of which there should be different subclasses such as NidaqManager and PulseStreamerManager. My preferred way of naming the general class would be DaqManager, as it will always be some form of general I/O DAQ board, and "signal generator" is to me a bit confusing as it's not necessarily a waveform generator.
  • I realized after we talked that the scanInfoDict is not only accessed by the APDManager, but also by the TTLCycleDesigners, who need the information to correctly assign the TTL lengths based on the pixel/line/frame lengths etc. This is also how the scanInfoDict originated, and then I decided to use the same dictionary to put some extra parameters for the APDManager. Therefore i am actually not sure that the best option is to remove this, as if so the APDManager would need to read info from the scanManager/scanWidget in some other way, and there might not be a better way to this.

@kasasxav
Copy link
Collaborator

kasasxav commented Jun 15, 2023

Hi, thank you for your message! I answer in points:

  • I agree with Jonatan that signal generator is not a thing in ImSwitch, we have signal designers that what they do is to create numpy arrays with "signals" (to send to the devices) from a set input of parameters which come from the ScanWidget/ScanController.
  • I don't think it's wrong at all to reference nidaqManager from APDManager, this is because that specific type of detector takes the pixels from the nidaq. Maybe it would be NidaqAPDManager, it's the same as the NidaqLaserManager. But since until now we haven't had thoughts on having more daqs that's the reason of the name.
  • The thing of the parameter compatibility @jonatanalvelid might know more because he did the SuperScanManager
  • I agree with the changes, since before we had that the analog signals were only for the scanners but it doesn't always have to be like this. As the scan widget was before this was the only way, but thanks to the work of Jonatan that made it abstract now one could do any scan they want :D
  • Agree with the scanInfoDict, that is the parameters of the scan (the signals). So it is not a problem
  • I'm not so convinced with the changes of the ScanAdvanced, I think there was a reason why things are called like this. But I don't remember at this point - I don't think it should be that important since it's not confusing in this case.
  • The last part, yes that what you have to do is to create the general DaqManager and children nidaqManager and pulseStreamerManager, and then have a section in the config file (SetupInfo) that you specify which of the daqs you want to use. I think I wanted to do this once.

Thank youu :)

P.S: the reason we wanted to do the daq stuff is because @Bodeeen wanted to use TrigerScope for the scans. In the end they ended up doing a different widget for that but the idea was to integrate it in the scan widget: did you do it in the end?

@pskeshu
Copy link

pskeshu commented Jul 13, 2023

hi,

I have been trying to use imswitch, which I love for the widgets and comms, but I think device layer and planning could be refactored out of the imswitch core to use best practices from third-party libraries which have more generalizable solutions.

For instance, the device architecture of ophyd https://nsls-ii.github.io/ophyd/architecture.html encompasses all design concerns regarding devices stay there, which are accessible by the planning layer https://nsls-ii.github.io/bluesky/plans.html

I have used this model in a personal project (1, 2), and I feel it helps a lot from a dev perspective to be able to rapidly add devices to the overall system and work with new or existing plans by writing plans through an established standard (https://nsls-ii.github.io/bluesky/plans.html#stub-plans).

Best,
Kesavan

@beniroquai
Copy link
Collaborator

beniroquai commented Jul 13, 2023 via email

@jacopoabramo
Copy link
Collaborator Author

jacopoabramo commented Jul 13, 2023

Hi @pskeshu , thanks for pitching in. I'm not quite sure I understand what you mean when you say "refactor out the device layer". Could you elaborate more on this?

Also, is there a

As for the links you posted I've only gave a glimpse and they present some really interesting concepts. What I'm afraid though in integrating all these abstraction layers on top of each other we may limit ourselves performance wise - especially when talking about camera acquisitions. But I may be mistaken since I'm still just reading the documentation.

Also how do I know if my device is supported? Is there a list of supported devices?

EDIT: I just realized you may have intended to say that the device layer should follow the model intended in ophyd rather than implementing ophyd in itself as a device layer replacement. Am I correct?

@jacopoabramo
Copy link
Collaborator Author

jacopoabramo commented Oct 8, 2023

@beniroquai to resuscitate this topic, for your stichting widget, do you think you would be able to adapt the suggestion for using ophyd as an abstraction level in cohesion with the scanning? As a test run to evaluate how hard or easy it is to use

@pskeshu
Copy link

pskeshu commented Oct 8, 2023

@jacopoabramo Sorry, I missed your EDIT. Yes, largely that's what I'm trying to articulate - that device layer can be refactored as ophyd objects, which can simplify support to a large extent. In principle then, any new device can be added to this architecture, as long as follows a standard like ophyd, which is very mature and has community support. There are so many advantages to this, that might warrant a call.

@jacopoabramo
Copy link
Collaborator Author

jacopoabramo commented Oct 8, 2023

@pskeshu all good, thanks any way for replying. Indeed after your post I dug more in depth into ophyd and it's a perfect fit for exactly what we want to achieve on the scanning side - and not only.

@nornil this is close to the generic experiment planner we were discussing in the past, since bluesky objective is exactly to plan and customize experiments

@pskeshu
Copy link

pskeshu commented Oct 8, 2023

Just to elaborate, ophyd support gives access to the larger bluesky ecosystem, which makes it easy to work across platforms. It creates standards for acquisition, planning, and overall looks really neat for writing experiments with python. And GUI integration with bluesky is an active project.

They might have insights on how to go about this. cc @tacaswell @danielballan

@jacopoabramo
Copy link
Collaborator Author

Thanks for the insight, @pskeshu . One thing I'm trying to wrap my mind around is mostly how performant the bluesky engine can be when trying to integrate high speed acquisitions. To elaborate: I'm currently using an hacked version of ImSwitch to control an interferometric scattering microscope (iSCAT) with a side fluorescence channel; the iSCAT channel uses a camera capable of reaching up to 10k FPS. As we use it for particle tracking experiments with exposure times as low as 300 us, the current recording engine allows me to only record a certain number of frames before losing them. Would bluesky be able to achieve such high framerates? If so, how? One thing I've been pondering is accelerating the model by compiling it with Cython; would this be a possibility for bluesky as well?

@pskeshu
Copy link

pskeshu commented Oct 8, 2023

A question for the nikea community perhaps - https://join.slack.com/t/nikea/shared_invite/zt-24yl9vom8-Dac~gmCsQkflInf4ayFVNw

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

No branches or pull requests

5 participants