cleanup morphologyHDF5 by mgeplf · Pull Request #285 · BlueBrain/MorphIO

mgeplf · 2021-04-15T12:51:04Z

make which datasets belong to which versions more clear
don't use try/catch to detect if datasets exist
try and clarify where member variables are read & saved
- removed member variables _points, _pointsDims, _sections,
  since their locality can be kept local
fix version numbering in tests/data/h5/v1/endoplasmic-reticulum.h5
mitochondria.h5
remove unnecessary morphio::

src/readers/morphologyHDF5.cpp

tomdele · 2021-04-16T09:53:09Z

src/readers/morphologyHDF5.cpp

+        if (metadata.hasAttribute(_a_version)) {
+            const auto attr = metadata.getAttribute(_a_version);
+
+            std::array<uint32_t, 2> versions;


This is not initialized and I am not familiar with HighFive but shouldn't we init to something like : 0, 0 ?

src/readers/morphologyHDF5.cpp

tomdele · 2021-04-16T09:53:14Z

src/readers/morphologyHDF5.cpp

-    if (vec.size() > 0) {
-        _points->read(vec.front().data());
+    if (hasNeurites) {
+        const size_t size = (points.size() + hd5fData.size() - section_offset);


same for points.size() it should/must be 0 no ?

tomdele · 2021-04-20T08:59:01Z

I made the changes I proposed in :
#288
it also fixed the no soma problem.

If you are ok with this PR above we can merge both.

asanin-epfl · 2021-04-21T17:45:31Z

Currently I have problems with hdf morphologies that have its soma points split among several offsets in structure:

HDF5 "scripts/three-point-soma-two-offset.h5" {
GROUP "/" {
   DATASET "points" {
      DATATYPE  H5T_IEEE_F32LE
      DATASPACE  SIMPLE { ( 8, 4 ) / ( 8, 4 ) }
      DATA {
      (0,0): -2, 0, 0, 1,
      (1,0): 0, 0, 0, 1,
      (2,0): 0, 0, 0, 1,
      (3,0): 2, 0, 0, 1,
      (4,0): 2, 0, 0, 1,
      (5,0): 3, 3, 0, 1,
      (6,0): 2, 0, 0, 1,
      (7,0): 4, 0, 4, 1
      }
   }
   DATASET "structure" {
      DATATYPE  H5T_STD_I32LE
      DATASPACE  SIMPLE { ( 4, 3 ) / ( 4, 3 ) }
      DATA {
      (0,0): 0, 1, -1,
      (1,0): 2, 1, -1,
      (2,0): 4, 3, 0,
      (3,0): 6, 2, 0
      }
   }
}
}

The 2nd half of soma is skipped and soma type is undefined.

three-point-soma-two-offset.h5
SomaType.SOMA_UNDEFINED
[[-2.  0.  0.]
 [ 0.  0.  0.]]
section points:
	 [2. 0. 0.]
	 [3. 3. 0.]
	 [2. 0. 0.]
	 [4. 0. 4.]

Should I be worried about this case when soma is represented by multiple offsets?

tomdele · 2021-04-22T08:26:31Z

Do we have specs on that ?
Having unifurcation on Neurites --> why not, but on Somas...

To me without more specs (like: " we can have only one soma"), this is an undefined behavior an we should raise because we cannot figure out if this is double soma morphology (one single point soma and one cylinder soma) or if this is a poorly formatted 3 points soma.

asanin-epfl · 2021-04-22T08:44:47Z

Do we have specs on that ?

There is something: "Soma: there is only one soma section, however, different tools interpret them differently:" (c). I propose to throw en exception for this case as we don't throw currently on the latest master branch.

tomdele · 2021-04-22T08:49:01Z

Ok so we throw. I will update the code

tomdele · 2021-04-22T17:26:05Z

Added a check here to ensure a single section soma and the soma being the first section:
f7c1c6a

* make which datasets belong to which versions more clear * don't use try/catch to detect if datasets exist * try and clarify where member variables are read & saved * removed member variables _points, _pointsDims, _sections, since their locality can be kept local * fix version numbering in tests/data/h5/v1/endoplasmic-reticulum.h5 mitochondria.h5 * remove unnecessary `morphio::`

* Fix the no soma h5 morphologies * wrong type for the structure * fix data again * Remove the size necessarily equal to zero. * remove cassert * Multiple sections soma

tomdele · 2021-04-23T13:47:13Z

I put you as a reviewer but this is more to know if you have something to add or comment @asanin-epfl

asanin-epfl · 2021-04-23T16:15:23Z

From my side looks fine. I'm working on docs update currently, so it's good for me. Also a question, should be a similar PR for the ASC reader?

* test for extra somas/out of order during traversal of sections * 100 % coverage * added explicit glia tests

tomdele · 2021-05-05T13:03:48Z

src/readers/morphologyHDF5.cpp

+    bool hasSoma = true;
+    if (static_cast<SectionType>(vec[0][SECTION_TYPE]) != SECTION_SOMA) {
+        hasSoma = false;
+    } else if (vec.size() == 1) {


indeed much better

tomdele

Looks good !

mgeplf · 2021-05-05T15:02:52Z

I also tested it w/ NeuroM d177237, as cross check.

mgeplf added the WIP Work in Progress label Apr 15, 2021

mgeplf force-pushed the cleanup_morphologyHDF5 branch from f857bf6 to e7fbd40 Compare April 15, 2021 12:52

mgeplf requested a review from tomdele April 16, 2021 08:51

tomdele reviewed Apr 16, 2021

View reviewed changes

src/readers/morphologyHDF5.cpp Show resolved Hide resolved

tomdele reviewed Apr 16, 2021

View reviewed changes

src/readers/morphologyHDF5.cpp Show resolved Hide resolved

tomdele reviewed Apr 16, 2021

View reviewed changes

src/readers/morphologyHDF5.cpp Outdated Show resolved Hide resolved

tomdele reviewed Apr 16, 2021

View reviewed changes

mgeplf and others added 2 commits April 23, 2021 11:23

Fix the no soma h5 morphologies (#288)

60f695a

* Fix the no soma h5 morphologies * wrong type for the structure * fix data again * Remove the size necessarily equal to zero. * remove cassert * Multiple sections soma

tomdele force-pushed the cleanup_morphologyHDF5 branch from 2a888cf to 60f695a Compare April 23, 2021 09:35

Add tests for somatypes

6ebb28b

tomdele requested a review from asanin-epfl April 23, 2021 13:46

asanin-epfl mentioned this pull request Apr 26, 2021

Revise specification documentation #294

Merged

mgeplf added 2 commits May 4, 2021 13:02

Don't copy whole array to find soma sections

5abcd9e

* test for extra somas/out of order during traversal of sections * 100 % coverage * added explicit glia tests

variable rename, use static_cast

9743596

mgeplf removed the WIP Work in Progress label May 4, 2021

mgeplf requested a review from tomdele May 5, 2021 12:56

tomdele reviewed May 5, 2021

View reviewed changes

tomdele approved these changes May 5, 2021

View reviewed changes

mgeplf merged commit 8e2a38f into master May 5, 2021

mgeplf deleted the cleanup_morphologyHDF5 branch May 5, 2021 15:04

Conversation

mgeplf commented Apr 15, 2021

Uh oh!

Uh oh!

Uh oh!

tomdele Apr 16, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tomdele Apr 16, 2021

Choose a reason for hiding this comment

Uh oh!

mgeplf Apr 28, 2021

Choose a reason for hiding this comment

Uh oh!

tomdele commented Apr 20, 2021

Uh oh!

asanin-epfl commented Apr 21, 2021

Uh oh!

tomdele commented Apr 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asanin-epfl commented Apr 22, 2021

Uh oh!

tomdele commented Apr 22, 2021

Uh oh!

tomdele commented Apr 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tomdele commented Apr 23, 2021

Uh oh!

asanin-epfl commented Apr 23, 2021

Uh oh!

tomdele May 5, 2021

Choose a reason for hiding this comment

Uh oh!

tomdele left a comment

Choose a reason for hiding this comment

Uh oh!

mgeplf commented May 5, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tomdele commented Apr 22, 2021 •

edited

Loading

tomdele commented Apr 22, 2021 •

edited

Loading