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.

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

Add OnePhotonSeries #523

Merged

Conversation

CodyCBakerPhD
Copy link
Contributor

@CodyCBakerPhD CodyCBakerPhD commented Jun 13, 2022

WIP following the discussion of #283

Summary of changes

  • Add OnePhotonSeries

Checklist

For all schema changes:

  • Add release notes for the PR to docs/format/source/format_release_notes.rst.

If this is the first schema change after a schema release (i.e., the version string in core/nwb.namespace.yaml does not
end in "-alpha"), then:

  • Update the version string in core/nwb.namespace.yaml and core/nwb.file.yaml to the next major/minor/patch
    version with the suffix "-alpha". For example, if the current version is 2.4.0 and this is a minor change, then the
    new version string should be "2.5.0-alpha".
  • Update the value of the version variable in docs/format/source/conf.py to the next version without the
    suffix "-alpha", e.g., "2.5.0".
  • Update the value of the release variable in docs/format/source/conf.py to the next version with the suffix
    "-alpha", e.g., "2.5.0-alpha".
  • Add a new section in the release notes docs/format/source/format_release_notes.rst for the new version
    with the date "Upcoming" in parentheses.

Resolves #283

core/nwb.ophys.yaml Outdated Show resolved Hide resolved
@CodyCBakerPhD CodyCBakerPhD changed the title Update nwb.ophys.yaml Add OnePhotonSeries Jun 13, 2022
@CodyCBakerPhD
Copy link
Contributor Author

Splitting this PR, which introduces the basic OnePhotonSeries object that has some extra optional fields the TwoPhotonSeries does not, with the rest of the discussion from #283 which focused on all the types of custom device metadata that might be needed if someone does not specify the manufacturer/model number (in the case of custom builds). I've stashed work on that kind of thing, though: https://github.com/catalystneuro/nwb-schema/tree/custom_optical_imaging_device

In the end, I think adding enhanced support for custom Devices like this is a separate topic from adding support for OnePhoton data itself, which is largely just a minor consideration of the differences with the TwoPhoton data.

It's also my opinion that going down the road of thoroughly documenting every possible custom device configuration parameter is so open-ended and dynamic that we should either only support free-form text (which is already supported by the description of any Device) for specifying as many of these details as the user wishes, or encourage the development of extensions for specific sub-types of devices. But I'm open to discussion on this, so I'll bring it up at the next meeting.

@CodyCBakerPhD CodyCBakerPhD marked this pull request as ready for review July 19, 2022 15:51
@CodyCBakerPhD
Copy link
Contributor Author

@oruebel @rly @bendichter Met with @DaveOC90 today who confirmed this new object captures and describes all the necessary components of their experiments.

PR is now ready for review.

core/nwb.ophys.yaml Outdated Show resolved Hide resolved
@oruebel
Copy link
Contributor

oruebel commented Jul 19, 2022

Overall the schema looks good to me. I just had a couple of minor questions/comments.

oruebel
oruebel previously approved these changes Jul 20, 2022
Copy link
Contributor

@oruebel oruebel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. We should ideally also get approval from @bendichter and @rly

@oruebel oruebel requested review from rly and bendichter July 20, 2022 15:55
core/nwb.ophys.yaml Outdated Show resolved Hide resolved
@CodyCBakerPhD
Copy link
Contributor Author

@oruebel @bendichter Are we good to merge this?

@CodyCBakerPhD
Copy link
Contributor Author

Field of view removed

@CodyCBakerPhD CodyCBakerPhD requested review from bendichter and removed request for rly September 20, 2022 16:35
@CodyCBakerPhD CodyCBakerPhD requested review from oruebel and bendichter and removed request for bendichter and oruebel September 20, 2022 16:35
oruebel
oruebel previously approved these changes Sep 20, 2022
@rly
Copy link
Contributor

rly commented Sep 20, 2022

Looks good. Please add and test a PyNWB API for this before we merge. This looks like a relatively simple data type. I wonder whether the get_class for this type in PyNWB is sufficient.

rly
rly previously approved these changes Sep 20, 2022
@bendichter
Copy link
Contributor

@CodyCBakerPhD I believe this would be a minor change, since it does not break backwards compatibility. Could you please update the changelog accordingly?

@CodyCBakerPhD CodyCBakerPhD dismissed stale reviews from rly and oruebel via b91a638 October 5, 2022 18:48
@CodyCBakerPhD
Copy link
Contributor Author

@bendichter Done, also included the subject attribute addition with it since they are 'new features added in a back-compatible manner'

@oruebel
Copy link
Contributor

oruebel commented Oct 24, 2022

one, also included the subject attribute addition

Where is this included here?

@CodyCBakerPhD
Copy link
Contributor Author

@oruebel oruebel merged commit e870091 into NeurodataWithoutBorders:dev Oct 24, 2022
@rly rly deleted the add_one_photon_series branch October 25, 2022 01:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DRAFT: Add OnePhotonSeries neurodata_type
4 participants