microscope-cockpit / cockpit

Cockpit is a microscope graphical user interface. It is a flexible and easy to extend platform aimed at life scientists using bespoke microscopes.
https://microscope-cockpit.org
GNU General Public License v3.0
37 stars 27 forks source link

Odd check in structuredIllumination.py #798

Closed thomasmfish closed 2 years ago

thomasmfish commented 2 years ago

Line 366 of structuredIllumination.py doesn't seem to make sense - the wavelength of the AmbientLight class is 0 (not a string), so longestWavelength in ['Ambient', 'ambient'] will never be true. I have had a stab at what I think it's supposed to be in a branch here but I'm not 100%.

carandraug commented 2 years ago

Looking at the history (this check was added in f0ffd9382d0c02e6094f0ff807253ef64b370001), it seems the check kinda made sense back in 2015. This is for two reasons:

  1. At the time the wavelength value could be a string (actually, it could be any python object). See an example config file sample-configs/lights.py@f0ffd9382d0c

  2. At the time we were still in Python 2 which would compare strings and ints:

>>> print(sys.version)
2.7.5 (default, Nov 16 2020, 22:23:17) 
[GCC 4.8.5 20150623 (Red Hat 4.8.5-44)]
>>> max(0, 'Ambient')
'Ambient'

>>> print(sys.version)
3.9.2 (default, Feb 28 2021, 17:03:44) 
[GCC 10.2.1 20210110]
>>> max(0, 'Ambient')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: '>' not supported between instances of 'str' and 'int'

On the other hand, because it sets longestWavelength to -50 after calling max, it seems that it only works if the ambient light is the first light in the list.

Anyway, I have rebased and squashed your fix. Closing as fixed.

thomasmfish commented 2 years ago

@carandraug Thanks!