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.

Sign up for GitHub

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

Jump to bottom

Change ophys.ImagingPlane fields excitation_lambda and imaging_rate from str to float #136

Closed
bendichter opened this issue Mar 31, 2018 · 5 comments · Fixed by NeurodataWithoutBorders/pynwb#697
Closed

Change ophys.ImagingPlane fields excitation_lambda and imaging_rate from str to float #136

bendichter opened this issue Mar 31, 2018 · 5 comments · Fixed by NeurodataWithoutBorders/pynwb#697
Assignees
@bendichter @ajtritt
Milestone
NWB 2.0 Full

Comments

@bendichter
Copy link
Contributor

bendichter commented Mar 31, 2018 •
edited
Loading

nwb-schema/core/nwb.ophys.yaml

Line 197 in a004202

dtype: text

Is there a reason why these are stored as strings? It seems more sensible to store them as floats.
@bendichter bendichter changed the title Change excitation_lambda field of ophys.ImagingPlane from float to str Change excitation_lambda field of ophys.ImagingPlane from str to float Mar 31, 2018
@bendichter bendichter changed the title Change excitation_lambda field of ophys.ImagingPlane from str to float Change ophys.ImagingPlane fields excitation_lambda and imaging_rate from str to float Mar 31, 2018
@ajtritt
Copy link
Member

ajtritt commented Apr 1, 2018

This got carried over from 1.x, but I agree, a float or a double would make more sense.

bendichter added a commit that referenced this issue Apr 2, 2018
@bendichter
change wavelength and excitation lambdas to float
d878a8c 
fix #136
@bendichter bendichter mentioned this issue Apr 2, 2018
Closed
@bendichter bendichter mentioned this issue Apr 21, 2018
Closed
5 tasks
@tjd2002 tjd2002 mentioned this issue Apr 26, 2018
Closed
@bendichter bendichter mentioned this issue Apr 26, 2018
Closed
@elyall
Copy link

elyall commented Apr 26, 2018

Also in all other classes, the sampling rate parameter is referred to as "rate". So I'd suggest changing the parameter's name ("imaging_rate" -> "rate") and the data type (str -> float).

@elyall
Copy link

elyall commented Apr 26, 2018

Alternatively, ImagingPlane's sampling rate parameter is redundant and can be removed altogether since all Series data tied to an ImagingPlane have to have their own rate/timestamps specified.

jcfr pushed a commit that referenced this issue Apr 28, 2018
@bendichter @jcfr
ophys/imaging_rate: use float instead of str
93a4cf2 
See #136
@jcfr jcfr mentioned this issue Apr 28, 2018
Closed
jcfr pushed a commit that referenced this issue Apr 28, 2018
@bendichter @jcfr
ophys/imaging_rate: use float instead of text
33fd130 
See #136
@oruebel oruebel added this to the NWB 2.0 Full milestone May 18, 2018
@bendichter
Copy link
Contributor Author

@oruebel @ajtritt how do you guys feel about removing imaging_rate from ImagingPlane as @elyall suggests? Might that cause issues I am not seeing?

@oruebel oruebel mentioned this issue Aug 24, 2018
Closed
@oruebel oruebel mentioned this issue Sep 12, 2018
Closed
5 tasks
@oruebel
Copy link
Contributor

oruebel commented Sep 12, 2018

This is fixed in NeurodataWithoutBorders/pynwb#612

@oruebel oruebel closed this as completed Sep 12, 2018
@bendichter bendichter mentioned this issue Nov 1, 2018
Merged
5 tasks
ajtritt added a commit that referenced this issue Aug 19, 2019
@ajtritt
fix #136
5003b56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@bendichter bendichter

@ajtritt ajtritt

Labels
None yet
Projects
None yet
Milestone
NWB 2.0 Full
4 participants
@bendichter @elyall @ajtritt @oruebel