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

Session description is set to empty string by default #887

Open
rly opened this issue Jun 29, 2024 · 2 comments
Open

Session description is set to empty string by default #887

rly opened this issue Jun 29, 2024 · 2 comments
Assignees
Labels
bug Something isn't working

Comments

@rly
Copy link
Collaborator

rly commented Jun 29, 2024

Describe the issue

NWBFile.session_description is a required dataset in the NWB schema. Going through the tutorial, this field is not set by default. During conversion, the session_description is set to "" (empty string). The NWB inspector does not complain about the session description not being provided. This results in Neurosift showing "Loading..." when reading a file converted by NWB GUIDE: flatironinstitute/neurosift#178

  1. Should the inspector recommend against setting the value to the empty string? I think yes. Just like the other "description" fields.
  2. Should this field be recommended to provide in GUIDE (colored yellow in the form)? I think yes.
  3. Should this field be required to provide in GUIDE (colored red in the form)? I'm not sure. I lean toward yes because it is required in the schema, and empty string is not informative. Alternatively, we decide that this field should not be required in the NWB schema.

How does NeuroConv set session_description?

@CodyCBakerPhD @bendichter

Steps to Reproduce

Convert a file using GUIDE, look at value of session_description

Operating System

Mac OS with Mac M1

GUIDE Version

1.0.1

Code of Conduct

Yes

Did you confirm this issue was not already reported?

Yes

@rly rly added the bug Something isn't working label Jun 29, 2024
@rly rly self-assigned this Jun 29, 2024
@bendichter
Copy link
Collaborator

Yes I would agree having an empty string is not good behavior. If I recall correctly, it might be difficult to make this optional in PyNWB without breaking backwards compatibility for ordered args to the NWBFile init. We switched everything to kwargs long ago and I'd be ok with breaking that at this point but that's the catch in making this optional.

For the GUIDE, I'd say let's make it required, matching the schema. Then let's consider making it optional in the schema separately.

@CodyCBakerPhD
Copy link
Collaborator

How does NeuroConv set session_description?

NeuroConv is the source of the current behavior, which makes it optional at a high level but because it's required to create a file, sets it to a default of an empty string (preferable to "no description" as happens automatically in a bunch of other places in NWB)

If the goal is to eventually make it optional to create a file, then it would be more work to make it temporarily required in GUIDE only to eventually undo that

Right now the momentum is still tilted in the direction of treating it as optional, so why not just take the next step and relax it at the schema level?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants