[Storage] Implement large-dataset storedisk semantics - #168
Conversation
|
Claude finished @suraj-ranganath's task in 4m 2s —— View job Code review — PR #168 [Storage] Large-dataset storedisk semantics
The core BlockingNone. Important
Nits
Test gaps
EEGLAB parity notes
Checked: correctness of |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| if isinstance(index, (list, tuple)): | ||
| datasets = [deepcopy(_dataset_at(alleeg, int(item))) for item in index] | ||
| return datasets, alleeg, [int(item) for item in index] | ||
| dataset = deepcopy(_dataset_at(alleeg, int(index))) | ||
| return dataset, alleeg, int(index) | ||
| indices = [int(item) for item in index] | ||
| if storedisk_enabled(): | ||
| offload_storedisk_datasets(alleeg, set()) | ||
| datasets = [deepcopy(_dataset_at(alleeg, item)) for item in indices] | ||
| return datasets, alleeg, indices |
There was a problem hiding this comment.
Important (correctness): The multi-index branch passes set() as current_indices, so offload_storedisk_datasets offloads every resident saved dataset — including the ones being retrieved. The returned datasets are therefore OffloadedData handles that raise RuntimeError on any [...] / np.asarray access.
This contradicts both the storedisk design (the current selection should stay resident) and the single-index branch right below, which keeps {current} resident and loads it via dataset_with_loaded_data. Concretely, EEGPrepSession.retrieve([2]) and retrieve(2) select the same dataset but the list form returns offloaded data while the scalar form returns resident data (session.py:219-220 chooses the branch purely on isinstance(indices, (list, tuple))).
Minimal fix is to keep the selected indices resident; ideally also load any that are currently offloaded, mirroring the scalar branch:
| if isinstance(index, (list, tuple)): | |
| datasets = [deepcopy(_dataset_at(alleeg, int(item))) for item in index] | |
| return datasets, alleeg, [int(item) for item in index] | |
| dataset = deepcopy(_dataset_at(alleeg, int(index))) | |
| return dataset, alleeg, int(index) | |
| indices = [int(item) for item in index] | |
| if storedisk_enabled(): | |
| offload_storedisk_datasets(alleeg, set()) | |
| datasets = [deepcopy(_dataset_at(alleeg, item)) for item in indices] | |
| return datasets, alleeg, indices | |
| if isinstance(index, (list, tuple)): | |
| indices = [int(item) for item in index] | |
| datasets = [dataset_with_loaded_data(_dataset_at(alleeg, item)) for item in indices] | |
| for item, dataset in zip(indices, datasets): | |
| if 1 <= item <= len(alleeg): | |
| alleeg[item - 1] = deepcopy(dataset) | |
| offload_storedisk_datasets(alleeg, set(indices)) | |
| return datasets, alleeg, indices |
There's currently no test for multi-index retrieve under option_storedisk=1 (the cases in tests/test_eeg_retrieve.py run with storedisk off), so this path is uncovered.
…ompletion' into phase/162-large-dataset-storage # Conflicts: # .notes/implementation-notes.html # src/eegprep/plugins/clean_rawdata/pop_clean_rawdata.py
1 participant
Add Python-native .fdt sidecar save/load, memory-mapped data handles, and storedisk offload/retrieve behavior across EEGPrepSession, pop_newset, STUDY save paths, and eegprep-console. Harden review-reported multi-index retrieval, memmap resave, and optional clean_rawdata browser behavior; documents storage semantics and Phase 5 matrix evidence.
Closes #162