Skip to content
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

Integrate aqnwb with nwb-format plugin #4

Draft
wants to merge 32 commits into
base: main
Choose a base branch
from

Conversation

stephprince
Copy link

@stephprince stephprince commented Sep 11, 2024

@jsiegle (cc @oruebel)

Here is an initial draft PR for the aqnwb integration with OpenEphys.

Current state

On my local computer (MacOS Apple Silicon), this current implementation will successfully build and install the nwb-format plugin and save valid NWB files when streaming example data from the FileReader source in the plugin-GUI.

There are several remaining to-do items that I've highlighted below, but we wanted to give you a chance to look at the code in more detail and give any feedback on what we have so far.

Summary of main changes:

  • added aqnwb to Source folder
    • currently we're just using a git submodule to add the .hpp/.cpp files and then compiling all the source files together. We will likely need to discuss the best approach long-term.
    • updated CMakeLists.txt to add Boost as a dependency
  • replaced the openFiles, closeFiles, writeContinuousData, and writeSpike functions in the NWBRecordEngine with aqnwb-based implementations
  • removed the NWBFormat.h/NWBFormat.cpp files (replaced by aqnwb classes)
  • added a NWBRecordEngine::createRecordingArrays function to facilitate mapping between OpenEphys channel structures and aqnwb channel information structures

TODO items:

Further information

API documentation can be found here. We've also been updating some user/developer tutorials to walk through the overall workflow and hopefully guide the integration process for other systems in the future.

@anjaldoshi anjaldoshi self-requested a review September 13, 2024 20:45
@anjaldoshi
Copy link
Collaborator

Hi @stephprince

I tested this PR on both a macOS (Apple Silicon) and a Windows 11 computer, and here are my findings:

macOS (Sonoma)

After installing Boost and generating build files with CMake, I encountered a build error when trying to compile the Xcode project. Specifically, the build failed with the following error from the aqnwb/tests directory:

nwb-format/Source/aqnwb/tests/examples/test_example.cpp:5:10: fatal error:
      'testUtils.hpp' file not found
#include "testUtils.hpp"
         ^~~~~~~~~~~~~~~
1 error generated.

The build process was unnecessarily attempting to compile the example code in the tests directory but could not locate the necessary header files. As a temporary workaround, I removed the tests directory, which allowed the plugin to build successfully.

For a proper fix, the CMakeLists.txt needs to be updated to prevent the tests from being built when building the plugin. It should either conditionally build the test files based on a CMake option or include the aqnwb/tests directory in the include_directories call to locate the required headers.

Once I addressed this issue, I successfully installed the plugin, loaded it into the GUI, and recorded sample data in NWB format without any problems. However, reading the saved data back into the GUI using the File Reader failed, which aligns with your note that integrating aqnwb into NWBFileSource is yet to be done.

Windows 11

Following the same procedure (install Boost, remove tests directory, generate build files using CMake), I encountered the following errors from the aqnwb source code when building the plugin. I couldn’t determine the exact cause of these errors, so I was unable to test data recording in NWB format using aqnwb on Windows.

Build FAILED.

       "C:\Users\anjal\Projects\OEPlugins\nwb-format\Build\ALL_BUILD.vcxproj" (default target) (1) ->
       "C:\Users\anjal\Projects\OEPlugins\nwb-format\Build\nwb-format.vcxproj" (default target) (3) ->
       (ClCompile target) ->
         C:\Users\anjal\Projects\OEPlugins\nwb-format\Source\aqnwb\src\spec\core.hpp(25,1): error C2026: string too big, trailing characters truncated [C:\Users\anjal\Projects
       \OEPlugins\nwb-format\Build\nwb-format.vcxproj]
         C:\Users\anjal\Projects\OEPlugins\nwb-format\Source\aqnwb\src\nwb\base\TimeSeries.cpp(75,39): error C2146: syntax error: missing ')' before identifier 'or' [C:\Users\
       anjal\Projects\OEPlugins\nwb-format\Build\nwb-format.vcxproj]
         C:\Users\anjal\Projects\OEPlugins\nwb-format\Source\aqnwb\src\nwb\base\TimeSeries.cpp(75,39): error C3861: 'or': identifier not found [C:\Users\anjal\Projects\OEPlugi
       ns\nwb-format\Build\nwb-format.vcxproj]
         C:\Users\anjal\Projects\OEPlugins\nwb-format\Source\aqnwb\src\nwb\base\TimeSeries.cpp(75,71): error C2059: syntax error: ')' [C:\Users\anjal\Projects\OEPlugins\nwb-fo
       rmat\Build\nwb-format.vcxproj]
         C:\Users\anjal\Projects\OEPlugins\nwb-format\Source\aqnwb\src\nwb\base\TimeSeries.cpp(76,5): error C2059: syntax error: 'return' [C:\Users\anjal\Projects\OEPlugins\nw
       b-format\Build\nwb-format.vcxproj]
         C:\Users\anjal\Projects\OEPlugins\nwb-format\Source\aqnwb\src\nwb\base\TimeSeries.cpp(77,5): error C2059: syntax error: 'else' [C:\Users\anjal\Projects\OEPlugins\nwb-
       format\Build\nwb-format.vcxproj]
         C:\Users\anjal\Projects\OEPlugins\nwb-format\Source\aqnwb\src\nwb\base\TimeSeries.cpp(77,10): error C2143: syntax error: missing ';' before '{' [C:\Users\anjal\Projec
       ts\OEPlugins\nwb-format\Build\nwb-format.vcxproj]
         C:\Users\anjal\Projects\OEPlugins\nwb-format\Source\aqnwb\src\nwb\base\TimeSeries.cpp(77,10): error C2447: '{': missing function header (old-style formal list?) [C:\U
       sers\anjal\Projects\OEPlugins\nwb-format\Build\nwb-format.vcxproj]
         C:\Users\anjal\Projects\OEPlugins\nwb-format\Source\aqnwb\src\nwb\base\TimeSeries.cpp(80,1): error C2059: syntax error: '}' [C:\Users\anjal\Projects\OEPlugins\nwb-for
       mat\Build\nwb-format.vcxproj]
         C:\Users\anjal\Projects\OEPlugins\nwb-format\Source\aqnwb\src\nwb\base\TimeSeries.cpp(80,1): error C2143: syntax error: missing ';' before '}' [C:\Users\anjal\Project
       s\OEPlugins\nwb-format\Build\nwb-format.vcxproj]

    0 Warning(s)
    10 Error(s)

I'll be happy to go over more details if needed. Also, let me know if there are specific steps required to test this PR that I'm not aware of.

@stephprince
Copy link
Author

Hi @anjaldoshi,

Thanks for testing and reporting your findings!

The build process was unnecessarily attempting to compile the example code in the tests directory but could not locate the necessary header files. As a temporary workaround, I removed the tests directory, which allowed the plugin to build successfully.

I see, I had attempted to use a sparse checkout of the git submodule to only pull the src directory and avoid this issue. However it seems the sparse checkout needs to be configured locally when you pull the repository. I’ll update the CMakeLists.txt file as you suggested to resolve this.

Once I addressed this issue, I successfully installed the plugin, loaded it into the GUI, and recorded sample data in NWB format without any problems.

Great to hear!

However, reading the saved data back into the GUI using the File Reader failed, which aligns with your note that integrating aqnwb into NWBFileSource is yet to be done.

Correct, we haven’t integrated aqnwb into NWBFileSource yet. We’re currently working on implementing read support for NWB files and plan to add the FileSource integration after.

Following the same procedure (install Boost, remove tests directory, generate build files using CMake), I encountered the following errors from the aqnwb source code when building the plugin.

I had only setup our CI workflows for macOS and ubuntu, so I haven't done any testing on Windows yet. I’ll work on adding Windows support to our CI pipeline so I can address those build errors. I'll let you know once that's ready!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants