Skip to content
This repository was archived by the owner on Feb 26, 2025. It is now read-only.

Parsing report SimulationConfig fields #194

Closed
wants to merge 7 commits into from
Closed

Parsing report SimulationConfig fields #194

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c027331
Parsing report SimulationConfig fields
jorblancoa Apr 21, 2022
29cd6ce
Clang format
jorblancoa Apr 21, 2022
0b32180
Add auxiliar functions to check validity of fields
jorblancoa Apr 28, 2022
7483916
Move aux functions to anonnymous namespace
jorblancoa Apr 28, 2022
d5c6b2b
Add default value for the field 'unit'
jorblancoa Apr 28, 2022
80fae08
Merge branch 'master' into jblanco/simconfig_report_parsing
mgeplf May 3, 2022
3bac0f4
Check correctness of comma separated values in variable_name
jorblancoa May 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions include/bbp/sonata/config.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,8 +205,21 @@ class SONATA_API SimulationConfig
struct Report {
/// Node sets on which to report
std::string cells;
/// Sections on which to report.
/// Possible values: "soma"(default), "axon", "dend", "apic", "all"
std::string sections;
/// Report type. Possible values: "compartment", "summation", "synapse"
std::string type;
/// For summation type, specify the handling of density values.
/// Possible values: "none", "area"(default)
std::string scaling;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have everything that looks like an enum be an enum, rather than using strings? I'm used to stringly typed things from python, but in c++ I think the style leans towards stronger typing.

@jorblancoa I see you were following the style of type above, but this is a good chance to change it if we want to.

@NadirRoGue do you have a feeling about this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My only concern is that since Neurodamus needs a string at the end, we will need to convert the enum to the string either in libsonata or later in neurodamus...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My only concern is that since Neurodamus needs a string at the end,

Can you point me to where neurodamus would need a string? The enums would be exposed, so being able to access them and compare that way should work.

Copy link
Contributor

@NadirRoGue NadirRoGue May 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If feasible, the enum would be less prone to mistakes and IDE-friendly, IMO

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course is possible, but the code would need to be changed in several places in Neurodamus/CoreNEURON
few examples:
https://bbpgitlab.epfl.ch/hpc/sim/neurodamus-py/-/blob/main/neurodamus/node.py#L884
https://bbpgitlab.epfl.ch/hpc/sim/neurodamus-py/-/blob/main/neurodamus/target_manager.py#L464
https://bbpgitlab.epfl.ch/hpc/sim/neurodamus-core/-/blob/main/hoc/Report.hoc#L86

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could pass an integer (the enum position) to hoc and from there parsing assuming we know which number corresponds to which type.

I was hoping there was something we could do that's more elegant - that seems error prone; is there a way we could put something in the hoc scope that would automatically have these values populated?

and later this is read from CoreNEURON.

iirc, CoreNEURON uses libsonata, so it would be able to take advantage of the the enums that were created there, so it would be safe to serialize only the enum int value.

Also we would need to translate in libsonata the string read [...]

Yeah, one has to write a toString/fromString; it's annoying boiler plate, but it's pretty much the standard for dealing with enums in C++ afaik.

The tradeoff I'm seeing is that we have to balance the current convenience of using only string with getting the safety of having enums, and having to do some refactoring in other locations. The latter could bring additional benefits, but those are hard to quantify at the moment. Am I reading that right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes exactly.
I created a draft PR also with some proof of concept on how it will look with enums
#197

I think is quite a bit of work in some places (specially hoc as we discussed) on top of having quite a bit extra lines of code aswell in libsonata.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the #197 looks like the right way to go - NLOHMANN_JSON_SERIALIZE_ENUM is a great find, didn't know about it.

I don't mind the extra lines in libsonata, especially if it gains us correctness and maintainability.

IIRC, pybind11 gives us __str__ for enums, so the initial burden of change over should be ok.

If it's ok with you, I'll take that PR, and augment it. In the meantime, I think we should be close to getting this in.

Copy link
Contributor Author

@jorblancoa jorblancoa May 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about this, however by default if you convert the enum to string you will get:

Type.compartment
Sections.soma
...

I was trying to overload the __str__ in the bindings to return just "compartment" or "soma", but is not as easy at it seems
pybind/pybind11#2537

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the enum value .name should work, no?

/// For compartment type, select compartments to report.
/// Possible values: "center"(default for sections: soma), "all"(default for other sections)
std::string compartments;
/// The simulation variable to access
std::string variableName;
/// Descriptive text of the unit recorded. Not validated for correctness
std::string unit;
/// Interval between reporting steps in milliseconds
double dt{};
/// Time to step reporting in milliseconds
Expand All @@ -215,6 +228,8 @@ class SONATA_API SimulationConfig
double endTime{};
/// Report filename. Default is "<report name>_SONATA.h5"
std::string fileName;
/// Allows for supressing a report so that is not created. Default is true
bool enabled = true;
};

/**
Expand Down
23 changes: 22 additions & 1 deletion python/bindings.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -539,9 +539,27 @@ PYBIND11_MODULE(_libsonata, m) {

py::class_<SimulationConfig::Report>(m, "Report", "List of parameters of a report")
.def_readonly("cells", &SimulationConfig::Report::cells, "Node sets on which to report")
.def_readonly("sections",
&SimulationConfig::Report::sections,
"Sections on which to report. "
"Possible values are 'soma', 'axon', 'dend', 'apic', 'all'")
.def_readonly("type",
&SimulationConfig::Report::type,
"Report type. Possible values are 'compartment', 'summation', 'synapse")
.def_readonly("scaling",
&SimulationConfig::Report::scaling,
"For summation type, specify the handling of density values. "
"Possible values are 'none', 'area'")
.def_readonly("compartments",
&SimulationConfig::Report::compartments,
"For compartment type, select compartments to report. "
"Possible values are 'center', 'all'")
.def_readonly("variable_name",
&SimulationConfig::Report::variableName,
"The simulation variable to access")
.def_readonly("unit",
&SimulationConfig::Report::unit,
"Descriptive text of the unit recorded")
.def_readonly("dt",
&SimulationConfig::Report::dt,
"Interval between reporting steps in milliseconds")
Expand All @@ -551,7 +569,10 @@ PYBIND11_MODULE(_libsonata, m) {
.def_readonly("end_time",
&SimulationConfig::Report::endTime,
"Time to stop reporting in milliseconds")
.def_readonly("file_name", &SimulationConfig::Report::fileName, "Report file name");
.def_readonly("file_name", &SimulationConfig::Report::fileName, "Report file name")
.def_readonly("enabled",
&SimulationConfig::Report::enabled,
"Allows for supressing a report so that is not created");

py::class_<SimulationConfig>(m, "SimulationConfig", "")
.def(py::init<const std::string&, const std::string&>())
Expand Down
9 changes: 9 additions & 0 deletions python/tests/test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -593,11 +593,20 @@ def test_basic(self):

self.assertEqual(self.config.report('soma').cells, 'Mosaic')
self.assertEqual(self.config.report('soma').type, 'compartment')
self.assertEqual(self.config.report('soma').compartments, 'center')
self.assertEqual(self.config.report('soma').enabled, True)
self.assertEqual(self.config.report('compartment').dt, 0.1)
self.assertEqual(self.config.report('compartment').sections, 'all')
self.assertEqual(self.config.report('compartment').compartments, 'all')
self.assertEqual(self.config.report('compartment').enabled, False)
self.assertEqual(self.config.report('axonal_comp_centers').start_time, 0)
self.assertEqual(self.config.report('axonal_comp_centers').compartments, 'center')
self.assertEqual(self.config.report('axonal_comp_centers').scaling, 'none')
self.assertEqual(self.config.report('axonal_comp_centers').file_name,
os.path.abspath(os.path.join(PATH, 'config/axon_centers.h5')))
self.assertEqual(self.config.report('cell_imembrane').end_time, 500)
self.assertEqual(self.config.report('cell_imembrane').scaling, 'area')
self.assertEqual(self.config.report('cell_imembrane').variable_name, 'i_membrane, IClamp')

self.assertEqual(self.config.network,
os.path.abspath(os.path.join(PATH, 'config/circuit_config.json')))
Expand Down
63 changes: 53 additions & 10 deletions src/config.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
*************************************************************************/

#include <bbp/sonata/config.h>
#include <bbp/sonata/optional.hpp>

#include <algorithm> // transform
#include <cassert>
Expand Down Expand Up @@ -129,6 +130,20 @@ std::string toAbsolute(const fs::path& base, const fs::path& path) {
return absolute.lexically_normal().string();
}

template <typename Type>
bool is_in(const Type& value, const std::initializer_list<Type>& lst) {
return (std::find(std::begin(lst), std::end(lst), value) != std::end(lst));
}

template <typename Type>
void checkValidField(const Type& field, const std::initializer_list<Type>& possibleValues) {
if (!is_in<std::string>(field, possibleValues)) {
throw SonataError(fmt::format("Field '{}' not supported ('{}') possible",
field,
fmt::join(possibleValues, ", ")));
}
}

} // namespace

class CircuitConfig::Parser
Expand Down Expand Up @@ -500,11 +515,17 @@ class SimulationConfig::Parser
buf = element->template get<Type>();
}

template <typename Iterator, typename Type>
void parseOptional(const Iterator& it, const char* name, Type& buf) const {
template <typename Type, typename Iterator>
void parseOptional(const Iterator& it,
const char* name,
Type& buf,
nonstd::optional<Type> default_value = nonstd::nullopt) const {
const auto element = it.find(name);
if (element != it.end())
if (element != it.end()) {
buf = element->template get<Type>();
} else if (default_value != nonstd::nullopt) {
buf = default_value.value();
}
}

SimulationConfig::Run parseRun() const {
Expand Down Expand Up @@ -546,17 +567,39 @@ class SimulationConfig::Parser
auto& valueIt = it.value();
const auto debugStr = fmt::format("report {}", it.key());
parseMandatory(valueIt, "cells", debugStr, report.cells);
parseOptional<std::string>(valueIt, "sections", report.sections, "soma");
parseMandatory(valueIt, "type", debugStr, report.type);
parseOptional<std::string>(valueIt, "scaling", report.scaling, "area");
if (report.sections == "soma") {
parseOptional<std::string>(valueIt, "compartments", report.compartments, "center");
} else {
parseOptional<std::string>(valueIt, "compartments", report.compartments, "all");
}
parseMandatory(valueIt, "variable_name", debugStr, report.variableName);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for variable_name, I see the spec says there is the possibility of comma separated values; it should probably be checked for correctness (ie: trailing ,, double comma, etc)

parseOptional<std::string>(valueIt, "unit", report.unit, "mV");
parseMandatory(valueIt, "dt", debugStr, report.dt);
parseMandatory(valueIt, "start_time", debugStr, report.startTime);
parseMandatory(valueIt, "end_time", debugStr, report.endTime);
parseOptional(valueIt, "file_name", report.fileName);
if (report.fileName.empty())
report.fileName = it.key() + "_SONATA.h5";
else {
const auto extension = fs::path(report.fileName).extension().string();
if (extension.empty() || extension != ".h5")
report.fileName += ".h5";
parseOptional<std::string>(valueIt,
"file_name",
report.fileName,
it.key() + "_SONATA.h5");
parseOptional(valueIt, "enabled", report.enabled);

checkValidField<std::string>(report.sections, {"soma", "axon", "dend", "apic", "all"});
checkValidField<std::string>(report.scaling, {"none", "area"});
checkValidField<std::string>(report.compartments, {"center", "all"});

// Validate comma separated strings
std::regex expr("^\\w+(\\s*,\\s*\\w+)*$");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC, the ^ and $ aren't needed for regex_match, since it matches the whole thing.

The grouping can be non-capturing, since we don't extract results.

Using a raw string literal will also mean the double backslashes aren't needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets close then this PR and continue the conversation in #197

if (!std::regex_match(report.variableName, expr)) {
throw SonataError(fmt::format("Invalid comma separated variable names '{}'",
report.variableName));
}

const auto extension = fs::path(report.fileName).extension().string();
if (extension.empty() || extension != ".h5") {
report.fileName += ".h5";
}
report.fileName = toAbsolute(_basePath, report.fileName);
}
Expand Down
7 changes: 4 additions & 3 deletions tests/data/config/simulation_config.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,7 @@
"dt": 0.1,
"start_time" : 0,
"end_time" : 500,
"file_name": "soma",
"enabled" : true
"file_name": "soma"
},
"compartment": {
"cells": "Mosaic",
Expand All @@ -37,7 +36,7 @@
"start_time" : 0,
"end_time" : 500,
"file_name": "voltage",
"enabled" : true
"enabled" : false
},
"axonal_comp_centers": {
"cells": "Mosaic",
Expand All @@ -46,6 +45,7 @@
"variable_name": "v",
"unit": "mV",
"compartments": "center",
"scaling": "none",
"dt": 0.1,
"start_time" : 0,
"end_time" : 500,
Expand All @@ -56,6 +56,7 @@
"cells": "Column",
"sections": "soma",
"type": "summation",
"variable_name": "i_membrane, IClamp",
"unit": "nA",
"dt": 0.05,
"start_time": 0,
Expand Down
106 changes: 106 additions & 0 deletions tests/test_config.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,12 +312,21 @@ TEST_CASE("SimulationConfig") {

CHECK(config.getReport("soma").cells == "Mosaic");
CHECK(config.getReport("soma").type == "compartment");
CHECK(config.getReport("soma").compartments == "center");
CHECK(config.getReport("soma").enabled == true);
CHECK(config.getReport("compartment").dt == 0.1);
CHECK(config.getReport("compartment").sections == "all");
CHECK(config.getReport("compartment").compartments == "all");
CHECK(config.getReport("compartment").enabled == false);
CHECK(config.getReport("axonal_comp_centers").startTime == 0.);
CHECK(config.getReport("axonal_comp_centers").compartments == "center");
CHECK(config.getReport("axonal_comp_centers").scaling == "none");
const auto axonalFilePath = fs::absolute(basePath / fs::path("axon_centers.h5"));
CHECK(config.getReport("axonal_comp_centers").fileName ==
axonalFilePath.lexically_normal());
CHECK(config.getReport("cell_imembrane").endTime == 500.);
CHECK(config.getReport("cell_imembrane").scaling == "area");
CHECK(config.getReport("cell_imembrane").variableName == "i_membrane, IClamp");

CHECK_NOTHROW(nlohmann::json::parse(config.getJSON()));
CHECK(config.getBasePath() == basePath.lexically_normal());
Expand Down Expand Up @@ -408,6 +417,43 @@ TEST_CASE("SimulationConfig") {
})";
CHECK_THROWS_AS(SimulationConfig(contents, "./"), SonataError);
}
{ // No variable_name in a report object
auto contents = R"({
"run": {
"dt": 0.05,
"tstop": 1000
},
"reports": {
"test": {
"cells": "nodesetstring",
"type": "typestring",
"dt": 0.05,
"start_time": 0,
"end_time": 500
}
}
})";
CHECK_THROWS_AS(SimulationConfig(contents, "./"), SonataError);
}
{ // Wrong variable_name in a report object
auto contents = R"({
"run": {
"dt": 0.05,
"tstop": 1000
},
"reports": {
"test": {
"cells": "nodesetstring",
"variable_name": "variablestring,",
"type": "typestring",
"dt": 0.05,
"start_time": 0,
"end_time": 500
}
}
})";
CHECK_THROWS_AS(SimulationConfig(contents, "./"), SonataError);
}
{ // No dt in a report object
auto contents = R"({
"run": {
Expand Down Expand Up @@ -462,5 +508,65 @@ TEST_CASE("SimulationConfig") {
})";
CHECK_THROWS_AS(SimulationConfig(contents, "./"), SonataError);
}
{ // Invalid sections in a report object
auto contents = R"({
"run": {
"dt": 0.05,
"tstop": 1000
},
"reports": {
"test": {
"cells": "nodesetstring",
"sections": "none",
"type": "typestring",
"variable_name": "variablestring",
"dt": 0.05,
"start_time": 0,
"end_time": 500
}
}
})";
CHECK_THROWS_AS(SimulationConfig(contents, "./"), SonataError);
}
{ // Invalid scaling in a report object
auto contents = R"({
"run": {
"dt": 0.05,
"tstop": 1000
},
"reports": {
"test": {
"cells": "nodesetstring",
"scaling": "linear",
"type": "typestring",
"variable_name": "variablestring",
"dt": 0.05,
"start_time": 0,
"end_time": 500
}
}
})";
CHECK_THROWS_AS(SimulationConfig(contents, "./"), SonataError);
}
{ // Invalid compartments in a report object
auto contents = R"({
"run": {
"dt": 0.05,
"tstop": 1000
},
"reports": {
"test": {
"cells": "nodesetstring",
"compartments": "middle",
"type": "typestring",
"variable_name": "variablestring",
"dt": 0.05,
"start_time": 0,
"end_time": 500
}
}
})";
CHECK_THROWS_AS(SimulationConfig(contents, "./"), SonataError);
}
}
}