Parsing report SimulationConfig fields #194
Conversation
|
@mgeplf: could you take a look at this again so that we can get this in? |
| parseMandatory(valueIt, "type", debugStr, report.type); | ||
| parseOptional(valueIt, "scaling", report.scaling); | ||
| parseOptional(valueIt, "compartments", report.compartments); | ||
| parseMandatory(valueIt, "variable_name", debugStr, report.variableName); |
There was a problem hiding this comment.
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)
| /// 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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
If feasible, the enum would be less prone to mistakes and IDE-friendly, IMO
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
the enum value .name should work, no?
Add default values as optional when parsing
| /// 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; |
There was a problem hiding this comment.
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.
| checkValidField<std::string>(report.compartments, {"center", "all"}); | ||
|
|
||
| // Validate comma separated strings | ||
| std::regex expr("^\\w+(\\s*,\\s*\\w+)*$"); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Lets close then this PR and continue the conversation in #197
No description provided.