catalystneuro / ndx-photometry

photometry extension for nwb
BSD 3-Clause "New" or "Revised" License
0 stars 1 forks source link

general improvements #14

Closed luiztauffer closed 7 months ago

luiztauffer commented 7 months ago
luiztauffer commented 7 months ago

@pauladkisson named the new fluorophores attributes excitation_lambda and emission_lambda to keep it consistent with Pynwb ophys module

pauladkisson commented 7 months ago

named the new fluorophores attributes excitation_lambda and emission_lambda to keep it consistent with Pynwb ophys module

Hmmm, I do think emission_peak_wavelength is a more informative name, and we are planning to propose an overhaul of microscopy in NWB (see NEP Microscopy). But, maybe its best to keep it consistent with ophys for now...curious what @alessandratrapani thinks.

luiztauffer commented 7 months ago

I agree that emission_peak_wavelength is a better name here, I'll be happy to change for that. But at the same time, it is the same property that's called emission_lambda in the current Ophys module, and it also seems to be the term used for NEP you shared... let's hear what @alessandratrapani and @weiglszonja think about it

alessandratrapani commented 7 months ago

named the new fluorophores attributes excitation_lambda and emission_lambda to keep it consistent with Pynwb ophys module

Hmmm, I do think emission_peak_wavelength is a more informative name, and we are planning to propose an overhaul of microscopy in NWB (see NEP Microscopy). But, maybe its best to keep it consistent with ophys for now...curious what @alessandratrapani thinks.

I would definitely go with wavelength instead of lambda. The choice between emission_peak or just emission depends on what information on the emission spectrum we want to store. The emission_peak_wavelength is an intrinsic property of the fluorophore, but the same fluorophore can be imaged through different optical filters that depend on the setup requirements (for example to avoid cross-talk with excitation wavelength or imaging multiple fluorophores). I would prefer to use emission_wavelentgh to store the emission wavelength that is read through the filter, since the emission peak wavelentgh can be easily retrieved from the indicator/opsin specified in the respective field. image

luiztauffer commented 7 months ago

thanks for the explanation @alessandratrapani I changed the fluorophores properties to xxx_peak_wavelength. We'll include filters information in a new minicube class, in a next PR. If everyone agrees with the current state, I think we should merge this @pauladkisson @weiglszonja

alessandratrapani commented 7 months ago

thanks for the explanation @alessandratrapani I changed the fluorophores properties to xxx_peak_wavelength. We'll include filters information in a new minicube class, in a next PR. If everyone agrees with the current state, I think we should merge this @pauladkisson @weiglszonja

Sorry Luiz, I think I explain myself poorly. What I meant was I would not use emission_peak_wavelentgh because that is intrinsic in the fluorophore type, I don't think there is the need to specify it. I would go for emission_wavelentgh and report the emitted wavelength read through the filter, which is usually reported in the specific of the acquisition system. Same for the excitation wavelength.

But if you want to report only the fluorophore characteristics here, and the acquisition system in another place, then I guess it's ok, even if I think it's redundant

luiztauffer commented 7 months ago

oh I see what you mean now

But if you want to characterize only the fluorophore characteristic here, and the acquisition system in another place, then I guess it's ok, even if I think it's redundant

yes, I feel it is important to have that property explicit within the Fluorophores items. We can then have the system's filter properties described in another class

luiztauffer commented 7 months ago

someone needs to send an approving review for the merging to be allowed