-
Notifications
You must be signed in to change notification settings - Fork 3
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
Update extension with optional columns #1
base: master
Are you sure you want to change the base?
Conversation
@@ -33,7 +33,7 @@ | |||
'install_requires': [ | |||
'pynwb>=1.3.0', | |||
'numpy<1.19.4', | |||
'h5py<3' | |||
'h5py', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bendichter
I couldn't install ndx-photometry
with h5py<3
, had legacy-install-failure
error for it.
The version that this installs with is 3.8.0
.
- name: frequency | ||
dtype: float | ||
doc: voltage frequency in unit hertz | ||
quantity: '?' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left this optional for now, but feels a duplication with rate
of TimeSeries
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this might be referring to the frequency of the alternating current, e.g. 60 Hz in the US and 50 Hz in Europe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but maybe not, I don't really remember
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, is it okay to leave it optional or rollback?
No description provided.