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

Create default processing modules #1944

Draft
wants to merge 2 commits into
base: dev
Choose a base branch
from
Draft

Create default processing modules #1944

wants to merge 2 commits into from

Conversation

rly
Copy link
Contributor

@rly rly commented Aug 4, 2024

Motivation

Fix #946. @bendichter let me know what you think.

With this PR, a new NWBFile will contain empty ProcessingModule objects named "ecephys", "icephys", "ophys", and "behavior" on initialization. A user can add to these ProcessingModules without needing to create them. Any ProcessingModules that are empty are not written to the file. So there is a minor discrepancy between what a user sees in the in-memory NWBFile object compared to the one written to disk.

To prevent breaking existing code that creates a ProcessingModule with one of these names (creating one that already exists will raise an error), I overrode the create_processing_module and add_processing_module methods to warn if it exists instead of raise an error.

@bendichter I initially liked your suggestion in #946 of implementing nwb.processing.behavior or nwb.processing["behavior"] to "return the ProcessingModule named behavior if it exists, and if it does not exist pynwb would automatically create that module and return it." However, I tried that and it results in creating a processing module named "behavior" simply when a user accesses it. This could happen when exploring a file or when editing a file in append mode. For example, accidentally calling nwb.processing["behavior"] would create an empty ProcessingModule that gets written to disk on io.write(). Now that is probably rare, but I think creating the item when get is called is less intuitive than having the modules automatically created on init and just not writing them if empty, but I'm not totally sure...

Alternative 2: we could create methods nwb.add_behavior_processing, nwb.add_ecephys_processing, etc. that create the ProcessingModule and add to it when called. Similar to nwb.add_trial. Downside: this is a new syntax that is different from other functions. But it avoids both issues above.

TODO: add tests

We can add these default ProcessingModule objects to the schema too https://github.com/NeurodataWithoutBorders/nwb-schema/tree/default_proc_mod but let's discuss the interface first.

How to test the behavior?

# create empty nwb file -- default ProcessingModules are created
nwbfile.processing["behavior"].add(time_series)  # this works without the user needing to create the "behavior" module first
for modules in nwbfile.processing:
    print(module)  # they exist
nwbfile.create_processing_module("ecephys", "description")  # this raises a warning because it already exists
io.write(nwbfile)  # empty ProcessingModules are not written

nwbfile = io.read()  # default ProcessingModules are created if they are not in the file

Checklist

  • Did you update CHANGELOG.md with your changes?
  • Have you checked our Contributing document?
  • Have you ensured the PR clearly describes the problem and the solution?
  • Is your contribution compliant with our coding style? This can be checked running ruff check . && codespell from the source directory.
  • Have you checked to ensure that there aren't other open Pull Requests for the same change?
  • Have you included the relevant issue number using "Fix #XXX" notation where XXX is the issue number? By including "Fix #XXX" you allow GitHub to close issue #XXX when the PR is merged.

@rly rly requested a review from bendichter August 4, 2024 07:26
@rly rly added this to the 3.0 milestone Oct 17, 2024
@rly
Copy link
Contributor Author

rly commented Oct 17, 2024

@bendichter what do you think about this change? This would be nice to include in PyNWB 3.0.

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.

offer default ProcessingModules named "behavior", "ecephys", etc.
1 participant