-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add "power" matching #147
add "power" matching #147
Conversation
@matz-e you're welcome to read this, I still have to up the coverage, and fix some of the build checks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, I think these are my initial 0.02 BTC. I've reached my mental call-graph reconstruction capacity, so I'm sorry if some of these don't make sense.
src/node_sets.cpp
Outdated
case Op::regex: | ||
return "$regex"; | ||
default: | ||
throw SonataError("Not Implemented NodeSetBasicOperator"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be formatted like above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I let clang-format work its magic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant with fmt::format
, sorry, not very clear.
use vector<bool> as suggested by Matthias
python/generated/docstrings.h
Outdated
@@ -75,6 +75,8 @@ Note: This does not match dynamics_params datasets)doc"; | |||
|
|||
static const char *__doc_bbp_sonata_NodePopulation_matchAttributeValues_2 = R"doc(Like matchAttributeValues, but for vectors of values to match)doc"; | |||
|
|||
static const char *__doc_bbp_sonata_NodePopulation_regexMatch = R"doc()doc"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No documentation?
Also, should we explicitly mark these as extensions to the SONATA™ spec? Or is there an upstream PR amending this situation already?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, need to add the doc...I will do that.
I think it's worth doing a PR upstream to the docs, at least so ppl can read about the feature
No description provided.