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 warning for non-.nwb file extensions in NWB conversion for Converters and Interfaces #1228

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

bendichter
Copy link
Contributor

fix #1212

@h-mayorquin
Copy link
Collaborator

What do you think of throwing an error instead of a warning?

Reason being that nwb is already throwing a warning here and the library should follow good practices, right?

@bendichter
Copy link
Contributor Author

Well I could imagine situations where someone might want to use another suffix. For example, they might want to have x.nwb.h5 and x.nwb.zarr to examine different compression strategies.

But maybe this would break dandi organize? I don't know.

There is not a huge advantage of also having the warning here. One small advantage is instead of on NWBHDF5IO is that the warning will occur before the conversion begins, which may take a long time.

@h-mayorquin
Copy link
Collaborator

Check this out:
NeurodataWithoutBorders/nwbinspector#567

If the convention of having nwb.zarr is embraced we will need extra logic for this.

Taking into account all of this discussion I am leaning to just outsource this to upstream libraries. They already have a warning on place and that way we don't need to add extra logic here.

Maybe we should update our dependencies to be at least the ones that have the warnings though.

@bendichter
Copy link
Contributor Author

yeah that warning is recent and I think was just released today. I'd be fine with relying on that

Copy link

codecov bot commented Feb 27, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.93%. Comparing base (fa8a16f) to head (140846f).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1228   +/-   ##
=======================================
  Coverage   89.92%   89.93%           
=======================================
  Files         131      131           
  Lines        8411     8417    +6     
=======================================
+ Hits         7564     7570    +6     
  Misses        847      847           
Flag Coverage Δ
unittests 89.93% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/neuroconv/basedatainterface.py 80.85% <100.00%> (+0.63%) ⬆️
src/neuroconv/nwbconverter.py 82.63% <100.00%> (+0.36%) ⬆️

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.

[Feature]: warning if the output nwbfile_path does not end in ".nwb"
2 participants