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

[Bug] NWBHDF5IO.can_read() fails for missing version #1919

Closed
h-mayorquin opened this issue Jun 13, 2024 · 3 comments · Fixed by #1934
Closed

[Bug] NWBHDF5IO.can_read() fails for missing version #1919

h-mayorquin opened this issue Jun 13, 2024 · 3 comments · Fixed by #1934
Assignees
Labels
category: bug errors in the code or code behavior priority: medium non-critical problem and/or affecting only a small set of NWB users
Milestone

Comments

@h-mayorquin
Copy link
Contributor

As in the title, the problem is the following, NWBHDF5IO.can_read() relies on get_nwbfile_version and critically it indexes the output:

return get_nwbfile_version(file)[1][0] >= 2 # Major version of NWB >= 2

However, get_nwbfile_version can also return None and then the indexing fails:

def get_nwbfile_version(**kwargs):

It seems to me that the solution should be that if get_nwbfile_version returns None then NWBHDF5IO.can_read() should return None instead of trying to index.

From the discussion in neuroconv:

catalystneuro/neuroconv#910

@CodyCBakerPhD
Copy link
Collaborator

then NWBHDF5IO.can_read() should return None instead of trying to index.

I'd say a function called can_read ought to just return -> bool, and so False in the case where no version was able to be found by conventional means

@h-mayorquin
Copy link
Contributor Author

Yeah, I think that makes sense. The only confusing semantics is that get_nwbfile_version returns None when the version can't be found. So maybe the file could be read but there is no version....

But I guess better be strict about it?

@stephprince
Copy link
Contributor

From what I understand, if the version can't be found the file is corrupt, not a valid NWBFile, or nothing has been written.

I think returning can_read as False makes sense - I can make a PR to update it so that it does not error if get_nwbfile_version returns None but returns False instead.

@stephprince stephprince self-assigned this Jun 14, 2024
@stephprince stephprince added category: bug errors in the code or code behavior priority: medium non-critical problem and/or affecting only a small set of NWB users labels Jun 14, 2024
@stephprince stephprince added this to the 2.8.0 milestone Jun 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: bug errors in the code or code behavior priority: medium non-critical problem and/or affecting only a small set of NWB users
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants