Skip to content

Add DeviceModel neurodata type #608

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

Open
wants to merge 7 commits into
base: dev
Choose a base branch
from
Open

Add DeviceModel neurodata type #608

wants to merge 7 commits into from

Conversation

rly
Copy link
Contributor

@rly rly commented Feb 5, 2025

Summary of changes

Fix #607

Checklist

For all schema changes:

  • Add release notes for the PR to docs/format/source/format_release_notes.rst.
  • 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.
  • Make sure that hdmf-common-schema points to the latest release and not the latest commit on the main branch.

@rly
Copy link
Contributor Author

rly commented Feb 9, 2025

In PyNWB, consider allowing the user to pass all devicemodel arguments to device constructor which creates a devicemodel behind the scenes with the same name + " Model" and links to it.

bendichter
bendichter previously approved these changes Feb 10, 2025
Copy link
Contributor

@bendichter bendichter left a comment

Choose a reason for hiding this comment

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

Lgtm. @oruebel what do you think?

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.

Just some minor enhancements to the changelog, otherwise this looks good. What is the plan for supporting this in PyNWB.

@oruebel
Copy link
Contributor

oruebel commented Feb 10, 2025

In PyNWB, consider allowing the user to pass all devicemodel arguments to device constructor which creates a devicemodel behind the scenes with the same name + " Model" and links to it.

Since the intent seems to be that folks will create extensions of DeviceModel, I'm wondering whether that approach may cause confusion. I'm wondering whether it may be simpler to have some factory method instead, e.g., create_device on NWBFile that would create both the Device and DeviceModel

@rly
Copy link
Contributor Author

rly commented Feb 20, 2025

In PyNWB, consider allowing the user to pass all devicemodel arguments to device constructor which creates a devicemodel behind the scenes with the same name + " Model" and links to it.

Since the intent seems to be that folks will create extensions of DeviceModel, I'm wondering whether that approach may cause confusion. I'm wondering whether it may be simpler to have some factory method instead, e.g., create_device on NWBFile that would create both the Device and DeviceModel

That makes sense. But factory methods will not work for extensions either if they require additional fields, so users would still have to create the DeviceModelExtension and DeviceExtension manually.

@rly rly requested a review from oruebel February 20, 2025 15:04
oruebel
oruebel previously approved these changes Feb 20, 2025
@rly rly changed the base branch from dev to band May 10, 2025 16:47
@rly rly mentioned this pull request May 10, 2025
Base automatically changed from band to dev May 28, 2025 22:17
@rly rly dismissed oruebel’s stale review May 28, 2025 22:17

The base branch was changed.

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.

Add DeviceModel neurodata type
3 participants