Skip to content

Commit 26d0dd0

Browse files
ehennestadehennestad
authored
Improve error messages when export of internal DataPipes fails (#718)
* Improve error messages when export of internal pipes fail * Update dataPipeTest.m Add unit tests for error messages * Fixed bug in new unittest * add more details to comments in new unittests * Unrelated: Fix value of spike_times description in test to match pynwb * Update BoundPipe.m Added third suggestion based on suggestion from @bendichter * Fix syntax error in unit test * Update BlueprintPipe.m Moved h5 cleanup to avoid code duplication
1 parent 61c21f1 commit 26d0dd0

File tree

3 files changed

+116
-4
lines changed

3 files changed

+116
-4
lines changed

‎+tests/+unit/dataPipeTest.m

Lines changed: 72 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -290,7 +290,78 @@ function testOverrideBoundPipeProperties(testCase)
290290
testCase.verifyEqual(ME.identifier, 'NWB:BoundPipe:CannotSetPipeProperty')
291291
end
292292
end
293-
293+
294+
function testBoundPipeExportToNewFileError(testCase)
295+
% Test error message when exporting bound DataPipe to new file
296+
297+
% Create original file with DataPipe
298+
originalFile = 'test_bound_original.nwb';
299+
newFile = 'test_bound_new.nwb';
300+
301+
nwb = tests.factory.NWBFile();
302+
303+
fData = randi(250, 10, 100);
304+
fData_compressed = types.untyped.DataPipe('data', fData);
305+
306+
fdataNWB = types.core.TimeSeries( ...
307+
'data', fData_compressed, ...
308+
'data_unit', 'mV', ...
309+
'starting_time', 0.0, ...
310+
'starting_time_rate', 30.0);
311+
312+
nwb.acquisition.set('test_data', fdataNWB);
313+
nwbExport(nwb, originalFile);
314+
315+
% Read the file (creates a bound DataPipe)
316+
file = nwbRead(originalFile, 'ignorecache');
317+
318+
% Try to export to new file - this should fail, because the
319+
% data pipe in the imported file object is a "bound" pipe (the data
320+
% is not in memory), and the bound pipe's write method can not
321+
% "pipe" the data into a new file.
322+
testCase.verifyError(@() nwbExport(file, newFile), ...
323+
'NWB:BoundPipe:CannotExportToNewFile');
324+
end
325+
326+
function testUnboundPipeExportToExistingFileError(testCase)
327+
% Test error message when exporting "unbound" DataPipe to existing file
328+
329+
existingFile = 'test_unbound_existing.nwb';
330+
331+
% Create first file with DataPipe
332+
nwb1 = tests.factory.NWBFile();
333+
334+
fData1 = randi(250, 10, 100);
335+
fData1_compressed = types.untyped.DataPipe('data', fData1);
336+
337+
fdataNWB1 = types.core.TimeSeries( ...
338+
'data', fData1_compressed, ...
339+
'data_unit', 'mV', ...
340+
'starting_time', 0.0, ...
341+
'starting_time_rate', 30.0);
342+
343+
nwb1.acquisition.set('test_data', fdataNWB1);
344+
nwbExport(nwb1, existingFile);
345+
346+
% Create second NWB object with same structure
347+
nwb2 = tests.factory.NWBFile();
348+
349+
fData2 = randi(250, 10, 100);
350+
fData2_compressed = types.untyped.DataPipe('data', fData2);
351+
fdataNWB2 = types.core.TimeSeries( ...
352+
'data', fData2_compressed, ...
353+
'data_unit', 'mV', ...
354+
'starting_time', 0.0, ...
355+
'starting_time_rate', 30.0);
356+
nwb2.acquisition.set('test_data', fdataNWB2);
357+
358+
% Try to export to existing file - this will fail, because a
359+
% dataset already exists in the acquisition/test_data/test
360+
% location.
361+
testCase.verifyError(@() nwbExport(nwb2, existingFile), ...
362+
'NWB:BlueprintPipe:DatasetAlreadyExists');
363+
end
364+
294365
function testShapeValidation(testCase)
295366
% Create a DataPipe with both maxSize and actual size that are
296367
% valid

‎+types/+untyped/+datapipe/BlueprintPipe.m

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -216,7 +216,30 @@ function removePipeProperty(obj, type)
216216
end
217217
dcpl = obj.makeDcpl();
218218
dapl = 'H5P_DEFAULT';
219-
did = H5D.create(fid, fullpath, tid, sid, lcpl, dcpl, dapl);
219+
220+
try
221+
did = H5D.create(fid, fullpath, tid, sid, lcpl, dcpl, dapl);
222+
catch ME
223+
% Clean up H5 id before throwing error
224+
H5P.close(dcpl);
225+
H5S.close(sid);
226+
if ~ischar(tid)
227+
H5T.close(tid);
228+
end
229+
230+
if contains(ME.message, "name already exists")
231+
% Improve error message if this fails because dataset
232+
% already exists.
233+
error('NWB:BlueprintPipe:DatasetAlreadyExists', ...
234+
['Cannot export an unbound DataPipe to an existing file that already ' ...
235+
'contains the dataset at path "%s". ' ...
236+
'To fix this: either export to a new file, or use export mode "overwrite" ' ...
237+
'to replace the existing file, or read the existing file and modify it in place.'], ...
238+
fullpath);
239+
else
240+
rethrow(ME);
241+
end
242+
end
220243

221244
H5D.close(did);
222245
H5P.close(dcpl);

‎+types/+untyped/+datapipe/BoundPipe.m

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -256,8 +256,26 @@ function removePipeProperty(~, ~)
256256
'Bound pipes cannot remove pipe properties.');
257257
end
258258

259-
function obj = write(obj, ~, ~)
260-
return;
259+
function obj = write(obj, fid, fullpath)
260+
% Check if the link exists
261+
exists = H5L.exists(fid, fullpath, 'H5P_DEFAULT');
262+
assert(exists, ...
263+
'NWB:BoundPipe:CannotExportToNewFile', ...
264+
['Cannot export the dataset (%s) to a new NWB file.\n', ...
265+
'The dataset you are trying to export is stored in a ', ...
266+
'different file ("%s"), and the data is not physically ', ...
267+
'included in the current NWB file object. When exporting to ', ...
268+
'a new file, NWB needs a complete copy of the data — but ', ...
269+
'in this case, it only has a reference to the data in the ', ...
270+
'original file.\n', ...
271+
'To fix this, you have three options:\n', ...
272+
'1. Export to the same file ("%s"), which already ', ...
273+
'contains the data.\n', ...
274+
'2. Load the data into memory and create a new dataset ' ...
275+
'that includes the full data, so it can be saved in the ', ...
276+
'new file.\n', ...
277+
'3. Copy the original file to a new file location and edit ', ...
278+
'the new file with MatNWB.'], fullpath, obj.filename, obj.filename);
261279
end
262280

263281
function data = load(obj, varargin)

0 commit comments

Comments
 (0)