AllenInstitute / MIES

Multichannel Igor Electrophysiology Suite
https://alleninstitute.github.io/MIES/user.html
Other
21 stars 6 forks source link

Fix nwb export of old MIES data < version 0.7 #2072

Closed MichaelHuth closed 2 months ago

MichaelHuth commented 3 months ago

I had t o patch some more locations in NWB to make it work with ancient LNB and current sweep format.

close #2040 close #2047

MichaelHuth commented 3 months ago

@t-b There is a bug, where the current code for Caching fails with data in old MIES caches. A possible fix would be to call CA_ClearCache() before any NWB export. However, I am unsure about the implications as it may imapct performance badly.

t-b commented 3 months ago

Calling CA_ClearCache() always works, but I'm curious to why a cache entry is broken.

Can't we do that implicitly? We do have versioned the cache keys for this reason.

Caching was also only introduced in version 1.0.

MichaelHuth commented 3 months ago

The Cache issue is a side effect of the test approach by loading root:MIES from the historic eperiment with LoadData recursively. The effect is that everythin is loaded, except WaveRef waves and the root:MIES:Cache:values is a wave ref wave. Because its missing after the load the NOTE_INDEX is not in sync anymore between keys and values, i.e. the cache is corrupted..

Funnily enough, if I use Merge Experiment all the waves are loaded. But I don't see a way to execute that programmatically.

t-b commented 3 months ago

Review:

c5d554b9 (Util: Add new mode to AreAllSingleSweepWavesPresent to check w/o backup, 2024-03-22)

Jeep.

945d76df (Util: On sweep upgrade also store original wave creation time, 2024-03-22)

+        sprintf newNote, "%s\r%s: %s\r", note(wv), SWEEP_NOTE_KEY_ORIGCREATIONTIME, modTimeStr

c26a7ab7 (Util: In SplitAndUpgradeSweep do not use tight existence check for singel channels, 2024-03-22)

f04a4d88 (Util: Add fallback for retrieving the correct column indices for the config wave, 2024-03-22)

d78c680d (NWB: Use a sweep wave getter that automatically upgrades the sweep if required, 2024-03-22)

978b4374 (NWB: Revise fallback for sweep creation time determination, 2024-03-22)

90e4f7de (Const: Introduce constant for freeroot DF name, 2024-03-26)

Fine.

663b6d05 (Const: Introduce string constant for MIES DF path, 2024-03-26)

Good.

b5738397 (Tests: Add test for direct exporting to NWB file, 2024-03-26)

Makes sense.

MichaelHuth commented 3 months ago

Can't we unify the format we feed into NWB_GetStartTimeOfSweep? This should only require to add a TextSweepToWaveRef call in NWB_FirstStartTimeOfAllSweeps.

Through the changes the time calculation moved to UpogradeSweep, such that NWB_GetStartTimeOfSweep got sweep wave format independent. If the LNB entry is not present it just attempts to read the time from the wave note, if that fails it asserts. The consistency for that to be fulfilled has to be guaranteed before as it is not possible to fix missing information there.