134 Adds hyper spectral imaging to qt3scan #140
Conversation
Squash to fully functional single commit or remove this commit. This commit adds the princeton spectrometer to the qt3scan application for testing purposed. It throws errors.
Adds spectrometer to qt3scan.main
This is not necessary. Tested to work *locally* with random data simulator. Needs to be tested in lab.
This property is not necessary for functionality of qt3scan. However, this doesn't mean a similar property isn't necessary for some daq components (NiDaqDigitalInputRateCounter). This has been tested on a local machine with random data simulator but must be tested on actual hardware before merging -- modify this commit message once testing is completed.
Currently stuck on the QT3ScanPrincetonSpectrometerController class failing to pass the isinstance check for QT3ScanDAQControllerInterface (qt3scan.main.py#L440 I've checked all the methods in QT3ScanPrincetonSpectometerController class but can't seem to find what's wrong!
Sidesteps qt3scan.main check for objects to pass isisntance checks of protocols. Instead, warnings are logged and the application continues. We do this now as a quick hack so we can continue to test the Princeton spectrometer integration. We must figure out why this isinstance returns False.
QT3ScanPrincetonSpectrometerController was failing the isinstance test for QT3ScanDAQControllerInterface. It's related to the Spectrometer's return type of exposure_time, I believe. Not entirely sure though. It's definitely the clock_rate property though.
pass isisntance check on QT3ScanDAQControllerInterface Wraps class to spectrometer.exposer_time in try block and prevents exceptions to propagate. This allows isinstance to return True when testing on local laptop without presense of LightField application or spectrometer.
pass isisntance check on QT3ScanDAQControllerInterface Wraps class to spectrometer.exposer_time in try block and prevents exceptions to propagate. This allows isinstance to return True when testing on local laptop without presense of LightField application or spectrometer.
…w/qt3-utils into 134sub-changes-to-interface
…w/qt3-utils into 134sub-changes-to-interface
pass isisntance check on QT3ScanDAQControllerInterface Wraps class to spectrometer.exposer_time in try block and prevents exceptions to propagate. This allows isinstance to return True when testing on local laptop without presense of LightField application or spectrometer.
…w/qt3-utils into 134sub-changes-to-interface
gadamc
force-pushed
the
134sub-changes-to-interface
branch
2 times, most recently
from
cd98022 to
27663fa
Compare
Creates two new interface classes to separate DAQs. One for DAQs that simply count: QT3ScanCounterDAQControllerInterface One for DAQs that acquire spectra: QT3ScanSpectrometerDAQControllerInterface Modifies the QT3ScanPrincetonSpectrometerController to adhere to QT3ScanSpectrometerDAQControllerInterface Implements QT3ScanHyperSpectralApplicationController without subclassing or utilizing CounterAndScanner Ports over some minor changes from mordi's branch 134sub-acquire-step-and-glue
Fixes bugs found in controller. Random Spectrometer works so far.
src/qt3utils/applications/controllers/princeton_spectrometer.py
Outdated
Show resolved
Hide resolved
The previous grating implementation was prone to bugs so this should allow the users to populate the dropdown with whatever gratings are available in their spectrometer.
Prevents file name collsions by setting each basefile name to a random string before each acquisition happens
src/qt3utils/applications/controllers/princeton_spectrometer.py
Outdated
Show resolved
Hide resolved
src/qt3utils/applications/controllers/princeton_spectrometer.py
Outdated
Show resolved
Hide resolved
when right-clicking on the application. Adds pixel index x and y values returned to callback functions.
There was a problem hiding this comment.
Overall, this is a good implementation of hyper spectral scans. I will submit a second round of reviews, once I see this in action today. One thing I would like to see is an explanation of who a person would go about adding their own spectrometer classes, and making this code easily accept different spectrometers.
| sleep(num_frames * acquisition_time_seconds) | ||
|
|
||
| while self.experiment.IsRunning: | ||
| sleep(0.1) # checking if the experiment is still running |
There was a problem hiding this comment.
This will cause issues if someone wants to abort the scan. best use only the while loop sleep function, and add a condition for an "abort" flag the use can pass any time during the measurement.
There was a problem hiding this comment.
@cmordi does SpectrometerDataAcquisition.stop_scan() allow a user to abort this?
There was a problem hiding this comment.
Abort the scan? Well that was the plan, when I went in the lab to test it today it stopped the scan but it would sometimes resume. Which is weird because I can see on the actual Lightfield application that the stop button on there is actually pressed. I was working on it when you sent this message.
There was a problem hiding this comment.
@gadamc Is it possible that your "Stop" functionality overrides mine?
| _t = self.spectrometer.exposure_time / 1000.0 #Convert from milliseconds to seconds. | ||
| except Exception as e: | ||
| self.logger.error(e) | ||
| _t = 2 #TODO: better default behavior. Should this be -1? 1? or should Spectrometer be changed. |
There was a problem hiding this comment.
would np.nan work better?
There was a problem hiding this comment.
Actually... I think I can remove this whole dumb thing. I copied this from the PrincetonSpectrometerDAQController. (But there's a good reason why it's done this way in that class.)
| _t = self.spectrometer.exposure_time / 1000.0 # Converting from milliseconds to seconds. | ||
| except Exception as e: | ||
| self.logger.error(e) | ||
| _t = 2 # TODO: better default behavior. Should this be -1? 1? or should Spectrometer be changed. |
There was a problem hiding this comment.
maybe np.nan is better?
There was a problem hiding this comment.
Not sure-- @cmordi we could revisit this and find out if this try block is still needed or if we fixed the problem. The problem relates to how the isinstance function of python works at this line:
. The isinstance checks to see if a class implements all of the methods in a Protocol and I think for properties, it actually runs the method due to some python import reason. But we could try to remove the try/except block and see if it works. If it doesn't work without the try block, then yeah maybe np.nan works...| def start(self) -> None: | ||
| """ | ||
| Nothing to be done in this method. All acquisition is happening in the "sample_spectrum" method. | ||
| """ | ||
| self.logger.debug('calling QT3ScanPrincetonSpectrometerController start') |
There was a problem hiding this comment.
Then why is it here? Or why not implement the sample_spectrum in here?
There was a problem hiding this comment.
@vasilisniaouris there's some detritus left-over from my initial pass at refactoring last fall. The definition of the interface requires this method. It could be better named to 'initialize_before_acquisition' perhaps. Or something. Depending on the long-term plans for this code, this would be a good cleanup task -- go through the interfaces and qt3scan, understand what's happening and then rename methods appropriately.
@vasilisniaouris more documentation is definitely needed. The README already had some documentation around implementing support for new hardware. We did need to make a fundamental code change to split the DAQ controller interface into one that supports counters and one that supports spectra. @cmordi we should document that in the readme. Having said that, I know there's pressure to release something. Given that documentation is usually something that doesn't get done, I highly recommend that we at least put something in the README that explains what's happening and how to implement a new spectrometer. It should be a natural extension of the information that is currently there. |
@gadamc Just updated the docs. One or two of the hyperlinks are for this branch which is obviously not correct but I will change those when we update the main branch. Please let me know if I included incorrect info in the docs |
Adds hyper spectral imaging to qt3scan --> #134