Add incomplete Sonata Circuit #222

karjonas · 2018-04-06T08:16:14Z

It only has support for getPositions and getRotations so far.

hernando · 2018-04-06T09:06:18Z

tests/data/sonata/simple_nodes.py

@@ -89,7 +93,7 @@ class ModelType:
    "etype": ["fast", "slow", "fast"]}

 writer = csv.writer(out, delimiter=' ',
-                    quotechar='|', quoting=csv.QUOTE_MINIMAL)
+                    quotechar='|', quoting=csv.QUOTE_MINIMAL, lineterminator='\n')


Why did you have to add lineterminator

It defaults to '\r\n' causing issues when parsing the csv file.

hernando · 2018-04-06T09:18:31Z

brain/detail/circuit.h

+    {
+        LBUNIMPLEMENTED;
+        return 0;
+    }


You can implement this function also

hernando · 2018-04-06T09:30:35Z

brain/detail/circuit.h

+        return Strings();
+    }
+
+    virtual URI getMorphologyURI(const std::string& /*name*/) const


For this function I've just realized that you can almost share the same implementation for all subclasses. You need to add a new virtual function getMorphologySource to replace the _morphologySource member variable in the original implementation.

hernando · 2018-04-06T09:44:44Z

brain/detail/circuit.h

+            output.push_back(rotations[gid]);
+        return output;
+    }
+    virtual Strings getMorphologyNames(const GIDSet& /*gids*/) const


This needs to be implemented

hernando · 2018-04-11T09:16:04Z

tests/sonata.cpp

+
+BOOST_AUTO_TEST_CASE(sonata_SonataConfig_constructors)
+{
+    brain::Circuit circuit(TEST_SONATA_SIMPLE_NETWORK_URI);


Check an exception is thrown if opening a json file which doesn't exist.

hernando · 2018-04-11T09:23:07Z

brain/detail/circuit.h

+    {
+        const auto& subnetworks = config.getNodes();
+        const auto& node = subnetworks.front();
+        brion::Nodes nodes(URI(basePath.string() + "/" + node.elements));


This is wrong, you're assuming that paths are relative, which may not be the case.
The CircuitConfig should deal with all path substitutions and return absolute paths so client code doesn't need to deal with this stuff. As a matter of fact, I missed that you haven't dealt with relative paths at all in the CircuitConfig implementation.

hernando · 2018-04-11T09:24:58Z

brain/detail/circuit.h

+
+        if (subnetworks.size() > 1)
+        {
+            LBWARN << "More than one subnetwork found, ignored." << std::endl;


More than one subnetwork found, ignoring extra ones.

hernando · 2018-04-11T09:25:13Z

brain/detail/circuit.h

+        const auto populations = nodes.getPopulationNames();
+        if (populations.size() > 1)
+        {
+            LBWARN << "More than one population found, ignored." << std::endl;


Same as above

hernando · 2018-04-11T09:28:03Z

brain/detail/circuit.h

+        if (nodeGroupIDs.size() > 1)
+        {
+            LBWARN << "More than one group ID found, ignored." << std::endl;
+        }


This dataset has as many elements as cells. You're always going to give a warning unless your circuit has 1 neuron.
What you must check is that all IDs are 0 and throw if there's any other value.

hernando · 2018-04-11T09:34:40Z

brain/detail/circuit.h

+    }
+
+    virtual URI getMorphologySource() const final { return morphologySource; }
+    virtual const brion::URI& getCircuitSource() const


The code for morphology caching needs to be reviewed. In principle I think this function should be renamed and return the circuit config .json path, but I have to think about it more carefully.

hernando · 2018-04-11T09:38:22Z

brain/detail/circuit.h

+
+        const auto names =
+            nodeGroup.getAttribute<std::string>("morphology_file", startIdx,
+                                                endIdx);


If this attribute doesn't exist, you have to look up in the CSV file based on the node type. This is exactly the case of ABI's circuits.

hernando · 2018-04-11T09:42:58Z

brain/detail/circuit.h

+    }
+    virtual Strings getMorphologyNames() const
+    {
+        return nodeGroup.getAttribute<std::string>("morphology_file");


UNIMPLEMENTED, the name is a bit misleading, it's not the morphology file names, it's the morphology type names. Which we don't have in ABI's circuits as far as I know.

hernando · 2018-04-11T09:45:56Z

brain/detail/circuit.h

+            }
+
+            expectedIdx++;
+        }


I would just compare nodeGroupIndices to a vector created with std::iota, it's shorter and it doesn't really matter which index is out of order because later we have to implement the indirect code.

I do not understand how to do that so I will leave like this since we are removing it anyway

hernando · 2018-04-11T09:48:53Z

brain/detail/circuit.h

@@ -222,12 +189,335 @@ class Circuit::Impl

    URI getMorphologyURI(const std::string& name) const
    {
-        URI uri(_morphologySource);
+        URI uri(getMorphologySource());


This is incorrect, but leave it like that I'll fix it later.

hernando

It's mostly OK, I'll take care of the remaining changes.

hernando · 2018-04-17T11:17:37Z

brain/detail/circuit.h

@@ -214,20 +183,384 @@ class Circuit::Impl

    virtual Vector3fs getPositions(const GIDSet& gids) const = 0;
    virtual size_ts getMTypes(const GIDSet& gids) const = 0;
-    virtual Strings getMorphologyNames() const = 0;
+    virtual Strings getMorphologyTypeNames() const = 0;
    virtual size_ts getETypes(const GIDSet& gids) const = 0;
    virtual Strings getElectrophysiologyNames() const = 0;


Rename this one also for consistency

hernando · 2018-04-17T11:18:47Z

brain/detail/circuit.h

+        if (boost::filesystem::path(name).is_absolute())
+            return URI(name);
+
+        URI uri(getMorphologySource());
        uri.setPath(uri.getPath() + "/" + name + ".h5");
        return uri;


I'll review this and change it myself. I will commit on top of your branch head.

hernando · 2018-04-17T11:20:17Z

brain/detail/circuit.h

+    virtual const brion::Synapse* getSynapseExtra() const = 0;
+    virtual const brion::Synapse& getSynapsePositions(
+        const bool afferent) const = 0;
+    virtual const URI& getSynapseSource() const = 0;


I think I'm going to do another some other refactorings here also. This design is looking terrible to me. This will also remove the need for the compilation hacks you made below.

hernando · 2018-04-17T11:24:41Z

brain/detail/circuit.h

+        const auto attributeNames = nodeGroup.getAttributeNames();
+        const bool hasMorpholgyFile =
+            std::find(attributeNames.begin(), attributeNames.end(),
+                      propertyName) != attributeNames.end();


What about adding a function to search for an attribute name in NodeGroup?

…nodes

hernando · 2018-04-25T12:54:26Z

retest this please

hernando · 2018-04-25T13:01:02Z

Retest this please

Renamed namespace of hlohmann::json to brion_nlohmann to prevent conflicts with tools linking to Brion and using a different version.

karjonas requested a review from hernando April 6, 2018 08:16

hernando suggested changes Apr 6, 2018

View reviewed changes

hernando reviewed Apr 11, 2018

View reviewed changes

hernando reviewed Apr 17, 2018

View reviewed changes

Jonas Karlsson added 26 commits April 23, 2018 13:35

Make common interface for BBPCircuit and coming sonata circuit

732449d

Implement SonataCircuit with stubs

982369d

Hack to avoid creating invalid variables

335e64b

Add empty constructor test

a1414e3

Update test data

9d6bc80

Add parsing of positions in Sonata circuit

2e69b87

Add rotation quaternion calculation

1e8023f

Add comments

f420742

Add comments

52a3eb2

Remove debug spam

b4f175b

Read only first subnetwork

aed461d

Read positions and rotations from datasets on demand

8a7e0e6

Read only needed slice of dataset

bfc9e1c

Read only needed slice of dataset

a2d8b72

Implement getNumNeurons() method

a7da8af

Merge common method

fe975c7

Implement getMorphologySource for Sonata

735b619

Use utf-8 string in test files to avoid issues

db63662

Implement getMorphologyNames

a91e680

Assert exception when opening nonexisting file

a732e28

Expand absolute paths in circuit config

d9aa8f5

Change error message

0672699

Fix incorrect logic for warning message

1783a96

Move variable initialization

12cac57

Cleanup temporary variable hack

d47b3ba

Make morphology name lookup in CSV-file if not present in h5 nodegroup

272d1c1

Jonas Karlsson added 5 commits April 23, 2018 13:35

Fix incorrect lookup of node_type_id dataset in nodeGroup instead of …

41f4b95

…nodes

Rename function to avoid confusion

fc8c569

Remove unused variable

b1dead3

Fix potential absolute path bug

7f69933

Workaround nullptr dereference compilation error

6c6bab4

hernando approved these changes Apr 23, 2018

View reviewed changes

Code refactorings

1765262

Renamed namespace of hlohmann::json to brion_nlohmann to prevent conflicts with tools linking to Brion and using a different version.

hernando merged commit d9e1352 into BlueBrain:master Apr 25, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add incomplete Sonata Circuit #222

Add incomplete Sonata Circuit #222

karjonas commented Apr 6, 2018

hernando Apr 6, 2018

karjonas Apr 6, 2018

hernando Apr 6, 2018

hernando Apr 6, 2018

hernando Apr 6, 2018

hernando Apr 11, 2018 •

edited

hernando Apr 11, 2018

hernando Apr 11, 2018

hernando Apr 11, 2018

hernando Apr 11, 2018

hernando Apr 11, 2018

hernando Apr 11, 2018

hernando Apr 11, 2018

hernando Apr 11, 2018

karjonas Apr 13, 2018

hernando Apr 11, 2018

hernando left a comment

hernando Apr 17, 2018

hernando Apr 17, 2018

hernando Apr 17, 2018

hernando Apr 17, 2018

hernando commented Apr 25, 2018

hernando commented Apr 25, 2018

Add incomplete Sonata Circuit #222

Add incomplete Sonata Circuit #222

Conversation

karjonas commented Apr 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hernando Apr 11, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hernando left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hernando commented Apr 25, 2018

hernando commented Apr 25, 2018

hernando Apr 11, 2018 •

edited