Conversion/MIES_MassExperimentProcessing.ipf: Fix it #2569
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
t-b
force-pushed
the
bugfix/2569-mass-conversion-pxp-to-nwb
branch
from
72b2422 to
cd41b0b
Compare
7 tasks
t-b
force-pushed
the
bugfix/2569-mass-conversion-pxp-to-nwb
branch
4 times, most recently
from
4ccd555 to
416f622
Compare
4 tasks
MichaelHuth
requested changes
Nov 25, 2025
Collaborator
MichaelHuth
left a comment
MichaelHuth
left a comment
There was a problem hiding this comment.
Good description in commits what each specific problem was.
I think the handling of export errors can be improved.
t-b
force-pushed
the
bugfix/2569-mass-conversion-pxp-to-nwb
branch
from
c703697 to
b9ce241
Compare
t-b
force-pushed
the
bugfix/2569-mass-conversion-pxp-to-nwb
branch
from
b9ce241 to
7c44b0f
Compare
Collaborator
Author
|
There was a problem hiding this comment.
Pull request overview
This PR implements comprehensive fixes for the Mass Experiment Processing feature, focusing on improved labnotebook handling, error management, and file organization. The changes ensure proper upgrade sequences, better error handling during NWB export, and introduce helper utilities for initial setup.
Key changes include:
- Enhanced labnotebook upgrade logic with proper checks for missing columns and dimension labels
- Improved error handling and history capture in mass experiment processing
- Addition of setup utilities and empty file creation for user procedures
- Refactored code to ensure upgrades occur before data access
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/clean_mies_installation.sh | Adds installation of conversion package to user procedures |
| Packages/tests/UTF_All_Includes.ipf | Includes mass experiment processing tests |
| Packages/tests/Basic/UTF_Labnotebook.ipf | Adds comprehensive labnotebook upgrade tests with edge cases |
| Packages/doc/index.rst | Updates IPNWB documentation reference |
| Packages/MIES/MIES_WaveDataFolderGetters.ipf | Enhances labnotebook upgrade logic with size checks and better empty detection |
| Packages/MIES/MIES_Utilities_Algorithm.ipf | Renames function parameters for clarity |
| Packages/MIES/MIES_NeuroDataWithoutBorders.ipf | Ensures upgrades run before export and adds data existence checks |
| Packages/MIES/MIES_MiesUtilities_Device.ipf | Improves device detection and replaces function call |
| Packages/MIES/MIES_DataBrowser.ipf | Moves labnotebook upgrade to device lock phase |
| Packages/MIES/MIES_DAEphys.ipf | Consolidates labnotebook upgrade to single function call |
| Packages/MIES/MIES_Constants.ipf | Bumps labnotebook version constant |
| Packages/MIES/MIES_Async.ipf | Renames function parameters for clarity |
| Packages/MIES/MIES_AnalysisFunctionHelpers.ipf | Renames function parameters to avoid shadowing |
| Packages/IPNWB | Updates subproject commit reference |
| Packages/Conversion/MIES_MassExperimentProcessing.ipf | Refactors error handling, adds file creation utility, and improves history capture |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
t-b
force-pushed
the
bugfix/2569-mass-conversion-pxp-to-nwb
branch
from
f3c58f7 to
c40a9c0
Compare
Collaborator
Author
|
I've started a mass conversion run from |
This matches the paths on the current developer machine of the author of this commit.
When we missed the labnotebook upgrade we get an invalid wave reference from GetLogbookKeysFromValues which is then passed into GetLogbookSettingsColumnFromSorted which calls BinarySearchText which fails with the below assertion. Let's add an explicit assertion as the below assertion is bogus. !!! Threadsafe assertion FAILED !!! Message: "Only works with 1D waves" Please provide the following information if you contact the MIES developers: ################################ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Stacktrace: AfterFileOpenHook(...)#L239 [MIES_MassExperimentProcessing.ipf] ProcessCurrentExperiment(...)#L110 [MIES_MassExperimentProcessing.ipf] PerformMiesTasks(...)#L154 [MIES_MassExperimentProcessing.ipf] NWB_ExportAllData(...)#L685 [MIES_NeuroDataWithoutBorders.ipf] NWB_GetFileForExport(...)#L190 [MIES_NeuroDataWithoutBorders.ipf] NWB_FirstStartTimeOfAllSweeps(...)#L89 [MIES_NeuroDataWithoutBorders.ipf] NWB_GetStartTimeOfSweep(...)#L49 [MIES_NeuroDataWithoutBorders.ipf] GetLastSettingTextIndep(...)#L308 [MIES_MiesUtilities_Logbook.ipf] GetLastSetting(...)#L590 [MIES_MiesUtilities_Logbook.ipf] GetLogbookSettingsColumn(...)#L558 [MIES_MiesUtilities_Logbook.ipf] GetLogbookSettingsColumnFromSorted(...)#L563 [MIES_MiesUtilities_Logbook.ipf] BinarySearchText(...)#L974 [MIES_Utilities_Algorithm.ipf]
The very first version of the labnotebook (prior to wave versioning) only had two columns. But some upgrade paths always assumed that it had more. Let's fix that by inserting the columns on non-matching name or if the column size is too small.
Due to IP bug #7699 global strings with the same name are picked up if the function reference returns a string. Let's just rename the function references to func to make it less likely to be a problem.
When the hardware folder root:MIES:HardwareDevices contains folders which don't follow our device naming scheme, we currently bug out. Avoid that by skipping the folder if it is not a device.
This is a code path which is triggered on NWB export. And we don't want to recreate the GUI state wave as that might fail for old versions. So let's just query the GUI control itself. A similiar fix was already applied in 4e62d8c (DAP: Change comment getter for user comment to retrieve string directly from control, 2025-04-02).
Then we can use it in TestMe() as well.
Since 72cb45a (NWB: Create separate NWB file for each device, 2024-08-13) we create separate NWB files, one for each device. This also means that NWB_ExportAllData asserts out if data from multiple devices is present and we have supplied a NWB path via outputFilePath. Instead of iterating over all devices and call NWB_ExportAllData for each device, which an earlier version of this patch did, we can also leverage overrideFileTemplate and let the file be internally derived.
A lot of the old existing experiments found on the machine of this developer had various file includes in the builtin procedure window. In order to make mass experiment processing and conversion possible let's create them empty so that compilation succeeds.
As we are doing the labnotebook upgrade now for every labnotebook we find, we want to be sure that we don't destroy anything. An easy test for that is to have the same initial labnotebook after throwing away the version. This already fails as we don't set the row dimension labels. Since forever we have set those only in ED_FindIndizesAndRedimension when adding entries. But it is actually cleaner to set them initially already.
This avoids having us to deal with the HistoryCarbonCopy notebook.
We now do all error handling inside the catch part, clear lingering RTEs before as usual and also cleanup left-over NWB files.
The DFREF getter does the upgrade from the old ITCDevices to HardwareDevices. So in case this exists, we need to upgrade the path before we use its string representation. While the current code is correct, it is less confusing in this way.
When we export data from multiple devices, we use NWB_FirstStartTimeOfAllSweeps() which in turn query the labnotebook. So we must be sure that we upgraded all labnotebooks from all devices already. But for upgrading the labnotebooks we also need to have finished datafolder renaming, so let's do that before as well.
…G returns nothing Some broken experiments have differing sweep data in the datafolder and the labnotebook. Let's add assertions in case we could not get the required single channel data.
Execute/P "LOADFILE" seems to have issues with UNC paths.
t-b
force-pushed
the
bugfix/2569-mass-conversion-pxp-to-nwb
branch
from
c40a9c0 to
10c5f48
Compare
Collaborator
Author
|
Will merge when CI has finished. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 14 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Collaborator
Author
|
@timjarsky Can you rubberstamp the PR here? So we can be merged later today. Thanks. |
timjarsky
approved these changes
Dec 10, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.