Skip to content
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

node_set rules population selection #157

Open
ferdonline opened this issue Sep 16, 2021 · 2 comments
Open

node_set rules population selection #157

ferdonline opened this issue Sep 16, 2021 · 2 comments

Comments

@ferdonline
Copy link
Contributor

ferdonline commented Sep 16, 2021

We don't seem to have a way to properly filter nodes according to the nodeset population selector..
The nodeset entries cannot be inspected to know which node population to open, and the Nodeset rule
is basically a no-op, which either return the same set if the provided node population matches, otherwise returns an empty set. We are left with the workaround of opening all the populations and run materialize against them all

if (std::find(values_.begin(), values_.end(), np.name()) != values_.end()) {

A possibility could be to have a somewhat high level API, where we could have NodeSet objects, inspect them, and (why not) having a NodeStorage doing filtering on entries directly from them.

Some extras:
I see constructors copying values passed by rvalue-ref, which is not very idiomatic. Either copy from const-ref of move the rvalue-ref.. (Unless you want to force passing only rvalues?)

explicit NodeSetBasicPopulation(std::vector<std::string>&& values)

Or even moving copies, instead of initializing from a const-ref
NodeSetCompoundRule(std::string name, CompoundTargets targets)

@mgeplf
Copy link
Contributor

mgeplf commented Sep 17, 2021

For the first point, that could be added - it's not something that we've run into, so feel free to open a PR.

With respect to 'Some Extras', if you see things that can be improved, I would put those in another ticket, and, even better, in PR, so we can see the changes.

@mgeplf
Copy link
Contributor

mgeplf commented Jan 6, 2022

@ferdonline: Any update on this? Are you planning on adding the functionality you need?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants