-
Notifications
You must be signed in to change notification settings - Fork 38
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
passes cache_spec=False to nwb_io.write() to avoid spec writer shape … #406
Conversation
Is it really required to not cache the spec? Caching the specification should always be done in order to ensure that the files can be validated later on. |
05a793f
to
27d7de3
Compare
@t-b When we write the file after adding the spike times, during the spec update in H5SpecWriter (https://github.com/hdmf-dev/hdmf/blob/dev/src/hdmf/backends/hdf5/h5_utils.py#L276) H5py's self.__group.require_dataset(name, shape=tuple(), data=data, dtype=self.__str_type) is called with the shape=tuple() argument. But the preexisting dataset has shape (1,), so it errors out instead keeping the old spec. This is just a temporary fix right now, and we do want to update the spec when we write nwb files. I'm opening an issue on pynwb. |
Will this change be used by ipfx.attach_metadata? |
@wbwakeman have skipping spec update in the attach_metadata module fixed the issue with metadata attachment? |
@sgratiy Yes, I can attach metadata now |
@MatthewAitken I could not see the issue open on pynwb as mentioned above. I wanted to link it to this issue. |
@MatthewAitken I discussed this with @t-b and he had some good thoughts on the issue. The NWB2 files that I am working with for the June release are being written by MIES and include a cached_specification already. When ipfx attach_metadata and append_spike_times write the NWB2 file, they are also attempting to cache the spec again. This causes the "Shapes do not match" error. This PR solves that problem by not caching the spec again, which leaves the one written by MIES. Another approach (thanks @t-b ) would be to remove the MIES one. That can be done with something like
What do you think about following this strategy to correct the problem? |
This may be unnecessary once this PR is merged? |
Resolve #404 …error