Closed talmo closed 2 years ago
Sounds great, and it was nice meeting you all today!
I just wanted to mention some technical details about operating on an NWBFile in append mode that we've learned from many past experiences...
The approach of
if Path(filename).exists() and not overwrite:
with NWBHDF5IO(filename, mode="r") as io:
nwb_file = io.read()
else:
nwb_file = NWBFile(
session_description=session_description,
identifier=identifier,
session_start_time=session_start_time,
)
# ...
with NWBHDF5IO(path, mode="w" if overwrite else "a") as io:
io.write(nwb_file)
when Path(filename).exists() = True
and overwrite = False
, the modified nwb_file
object after the # ...
stages will be pointing to the now-closed io
from when it was first read from with NWBHDF5IO(filename, mode="r") as io:
and this could cause issues when you finally hit the final io.write
in the new io
from with NWBHDF5IO(path, mode="w" if overwrite else "a") as io:
in the final step.
Instead, the way we've successfully performed appending is to do so from within the respective context that that nwb_file
object that was read.
Here are some code snippets that have evolved over the years for this purpose...
The oldest, probably least fancy way: https://github.com/catalystneuro/neuroconv/blob/cc1c2fb9b175eeabe026066901d2a8344585343a/src/nwb_conversion_tools/nwbconverter.py#L168-L181
which is basically what you have now except everything is combined into a single context and the # ...
steps are performed inside that context.
The newer/fancier way we have is through our own custom designed context for making the 'append' vs. 'make a fresh NWBFile' decision behind the scenes: https://github.com/catalystneuro/neuroconv/blob/main/src/neuroconv/nwbconverter.py#L173-L184 + https://github.com/catalystneuro/neuroconv/blob/main/src/neuroconv/tools/nwb_helpers.py#L118-L177
Of course another alternative is to simply not use a context at all and just remember to call io.close()
either at the end of the export or in an except
clause wrapper in the event that there was an error anywhere in the process. I personally prefer contexts for maximum safety, though.
Oh, and another design principle I'd strongly suggest is making whatever the steps are within that # ...
block an external function like add_content_to_nwbfile_object(nwbfile: pynwb.NWBFile, ...)
so that all the SLEAP/ndx-pose details can be added to an in-memory NWBFile object separate from all the I/O handling. Such a helper function is probably what we would integrate directly with the corresponding DataInterface in NeuroConv since we typically like to handle the integration of multiple data streams all in a single io.write()
call at the end of our process.
Anyway, looking forward to meeting some of you at Janelia next week! Cheers, Cody
Yet another detail that I just recalled - any call to NWBHDF5IO(filename, mode="r")
will also want to include the keyword argument load_namespaces=True
otherwise it might fail to open an existing file that has extensions used within it.
Excellent, thanks for the pointers @CodyCBakerPhD!! We'll be mindful of how we work with the IO handles.
We should support the features mentioned by @bendichter in our NWB I/O.
Make the adapter more flexible. https://github.com/talmolab/sleap/blob/d522f1d9df1f65d0911fa07e8b2636b182d03c0c/sleap/io/format/ndx_pose.py#L167
This should accept a couple more things to look like:
Create a high-level export API as a instance-level method on
Labels
with signature:Discussed in https://github.com/talmolab/sleap/discussions/851