Move ElectrodesTable creation #161

stephprince · 2025-02-20T17:34:39Z

Fix #155 by moving the creation of ElectrodesTable to a separate convenience function.

As part of these changes, I added some additional tests, a helper function for getting the size of a dataset, and a check for creating ElectricalSeries with electrodes indices outside of the possible range of the ElectrodesTable.

docs/pages/userdocs/workflow.dox

codecov-commenter · 2025-02-20T17:42:51Z

Codecov Report

Attention: Patch coverage is 96.15385% with 8 lines in your changes missing coverage. Please review.

Project coverage is 93.96%. Comparing base (40fe1dc) to head (e3ef713).

Files with missing lines	Patch %	Lines
src/nwb/NWBFile.cpp	77.77%	8 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #161      +/-   ##
==========================================
- Coverage   93.98%   93.96%   -0.02%     
==========================================
  Files          64       64              
  Lines        4952     5118     +166     
  Branches      273      274       +1     
==========================================
+ Hits         4654     4809     +155     
- Misses        288      299      +11     
  Partials       10       10

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

docs/pages/userdocs/workflow.dox

oruebel · 2025-02-20T17:58:27Z

src/io/BaseIO.hpp

+  /**
+   * @brief Returns the size of the dataset for each dimension.
+   * @param path The location in the file of the dataset.
+   * @return The dataset size.
+   */
+  virtual std::vector<SizeType> getDatasetSize(const std::string path) = 0;


It would be nice to have this function support both datasets and attributes. In that case, I would rename it to getStorageObjectShape.

It would be nice to also add the following function to ReadDataWrapper to make this new function convenient to use on read

inline std::vector<SizeType> getShape() const { return m_io->getShape(m_path); } - We should be consistent in how we use `shape` and `size`. I think we should use `shape` to refer to the sizes along all dimensions and `size` to refer to the total number of elements.

It would be nice to have this function support both datasets and attributes. In that case, I would rename it to getStorageObjectShape.
It would be nice to also add the following function to ReadDataWrapper to make this new function convenient to use on read

That makes sense, I will add functionality for attributes and use shape to refer to sizes along all dimensions

oruebel · 2025-02-20T18:48:56Z

src/nwb/NWBFile.cpp

@@ -28,7 +28,7 @@ constexpr SizeType CHUNK_XSIZE =
 constexpr SizeType SPIKE_CHUNK_XSIZE =
    8;  // TODO - replace with io settings input

-std::vector<SizeType> NWBFile::emptyContainerIndexes = {};
+std::vector<SizeType> NWBFile::m_emptyContainerIndexes = {};


Maybe Types.h is a better place for this. Alternatively, it seems that m_emptyContainerIndexes is only used to set default arguments of functions, so we could be explicit and just use "std::vector()" as default argument, i.e., use:

Status createAnnotationSeries( std::vector<std::string> recordingNames, RecordingContainers* recordingContainers = nullptr, std::vector<SizeType>& containerIndexes = std::vector<SizeType>());

If I remember correctly, I think I set this up this way because you cannot pass a temporary empty initializer list as a default argument to a non constant reference parameter. Creating the empty vector in advance was a solution for that. Another potential option is to change the parameter to an rvalue reference, but then I think there might be additional complications in terms of transferring ownership.

Co-authored-by: Oliver Ruebel <[email protected]>

stephprince · 2025-02-22T00:49:04Z

Based on our discussion, I made containerIndexes a required argument and removed the static m_emptyContainerIndexes variable. I think recordingContainers should likely also be required as an input for the user so I removed the default value for that parameter as well.

These changes will now also fix #162, if we want to take a different approach or discuss that particular issue more I can move the changes to getStorageObjectsShape to a separate PR.

stephprince · 2025-02-22T01:10:43Z

@oruebel this is ready for you to re-review

oruebel · 2025-02-22T03:42:13Z

docs/pages/userdocs/workflow.dox

@@ -14,12 +14,15 @@
 *    used for managing \ref AQNWB::NWB::Container "Container" objects for storing recordings.
 * 3. Create the \ref AQNWB::NWB::NWBFile "NWBFile" object used for managing and creating NWB
 *    file contents.
- * 4. Create the \ref AQNWB::NWB::Container "Container" objects (e.g.,
+ * 4. Create the recording metadata stored in ``\general`` in NWB, e.g., the  


Suggested change

* 4. Create the recording metadata stored in ``\general`` in NWB, e.g., the

* 4. Create the recording metadata stored in ``/general`` in NWB, e.g., the

oruebel · 2025-02-22T03:45:00Z

src/io/BaseIO.hpp

+   * @brief Returns the size of the dataset for each dimension.
+   * @param path The location in the file of the dataset.
+   * @return The dataset shape.


Suggested change

* @brief Returns the size of the dataset for each dimension.

* @param path The location in the file of the dataset.

* @return The dataset shape.

* @brief Returns the size of the dataset or attribute for each dimension.

* @param path The location of the dataset or attribute in the file

* @return The shape of the dataset or attribute.

oruebel · 2025-02-22T03:45:32Z

src/io/hdf5/HDF5IO.hpp

+  /**
+   * @brief Returns the size of the dataset for each dimension.
+   * @param path The location in the file of the dataset.
+   * @return The dataset shape.
+   */


Suggested change

/**

* @brief Returns the size of the dataset for each dimension.

* @param path The location in the file of the dataset.

* @return The dataset shape.

*/

* @brief Returns the size of the dataset or attribute for each dimension.

* @param path The location of the dataset or attribute in the file

* @return The shape of the dataset or attribute.

oruebel · 2025-02-22T03:49:27Z

src/nwb/NWBFile.cpp

+  }
+
+  // write electrodes information to datasets
+  // (requires that ElectrodeGroup data is written)


If the ElectrodeGroup data must be written, should this function then call m_io->flush() here first?

oruebel · 2025-02-22T03:52:41Z

I added a couple of minor comments (mostly updates to docs), but otherwise this looks good.

Note, there will be conflicts with #163

stephprince added 8 commits February 19, 2025 16:44

move electrodes table creation to separate function

742e31e

update workflows for electrodes table creation

6b4b62f

add helper function for dataset size

39ae551

add status checks, warnings for invalid electrodes values

3128d22

add electrodes table tests

e6225e5

update workflow documentation

830aea1

fix formatting

fe20dd2

use dims from dataset

6146a72

oruebel reviewed Feb 20, 2025

View reviewed changes

docs/pages/userdocs/workflow.dox Outdated Show resolved Hide resolved

oruebel reviewed Feb 20, 2025

View reviewed changes

docs/pages/userdocs/workflow.dox Outdated Show resolved Hide resolved

oruebel reviewed Feb 20, 2025

View reviewed changes

stephprince and others added 3 commits February 20, 2025 16:28

Update docs/pages/userdocs/workflow.dox

ffdcde3

Co-authored-by: Oliver Ruebel <[email protected]>

Update docs/pages/userdocs/workflow.dox

4c4ce2d

Co-authored-by: Oliver Ruebel <[email protected]>

update test filenames for non nwb files

410da4a

stephprince mentioned this pull request Feb 22, 2025

Type and size of array attributes #162

Open

stephprince added 5 commits February 21, 2025 16:43

update getShape function for attributes and datasets

6455265

add tests and fix attr array write

4c71030

remove static containerIndices vector and require

ac123bd

update tests

3c0b98e

fix typos in workflow docs

e3ef713

stephprince marked this pull request as ready for review February 22, 2025 01:10

oruebel reviewed Feb 22, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move ElectrodesTable creation #161

Move ElectrodesTable creation #161

stephprince commented Feb 20, 2025

codecov-commenter commented Feb 20, 2025 •

edited

Loading

oruebel Feb 20, 2025

stephprince Feb 21, 2025

oruebel Feb 20, 2025 •

edited

Loading

stephprince Feb 21, 2025

stephprince commented Feb 22, 2025

stephprince commented Feb 22, 2025

oruebel Feb 22, 2025

oruebel Feb 22, 2025

oruebel Feb 22, 2025

oruebel Feb 22, 2025

oruebel commented Feb 22, 2025

	* 4. Create the recording metadata stored in ``\general`` in NWB, e.g., the
	* 4. Create the recording metadata stored in ``/general`` in NWB, e.g., the

Move ElectrodesTable creation #161

Are you sure you want to change the base?

Move ElectrodesTable creation #161

Conversation

stephprince commented Feb 20, 2025

codecov-commenter commented Feb 20, 2025 • edited Loading

Codecov Report

oruebel Feb 20, 2025

Choose a reason for hiding this comment

stephprince Feb 21, 2025

Choose a reason for hiding this comment

oruebel Feb 20, 2025 • edited Loading

Choose a reason for hiding this comment

stephprince Feb 21, 2025

Choose a reason for hiding this comment

stephprince commented Feb 22, 2025

stephprince commented Feb 22, 2025

oruebel Feb 22, 2025

Choose a reason for hiding this comment

oruebel Feb 22, 2025

Choose a reason for hiding this comment

oruebel Feb 22, 2025

Choose a reason for hiding this comment

oruebel Feb 22, 2025

Choose a reason for hiding this comment

oruebel commented Feb 22, 2025

codecov-commenter commented Feb 20, 2025 •

edited

Loading

oruebel Feb 20, 2025 •

edited

Loading