New module which filters on logical expressions of Paths #20028
Conversation
|
A new Pull Request was created by @wddgit (W. David Dagenhart) for master. It involves the following packages: FWCore/Modules @cmsbuild, @smuzaffar, @Dr15Jones can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
please test |
|
The tests are being triggered in jenkins. |
| virtual bool filter(StreamID, Event&, EventSetup const&) const override final; | ||
|
|
||
| private: | ||
| std::unique_ptr<pathStatusExpression::Evaluator> evaluator_; |
There was a problem hiding this comment.
Please see of edm::propagate_const can be used here.
| } | ||
|
|
||
| private: | ||
| std::unique_ptr<Evaluator> operand_; |
There was a problem hiding this comment.
std::propagate_const here as well.
| std::unique_ptr<Evaluator> operand_; | ||
| }; | ||
|
|
||
| class AndOperator : public Evaluator { |
There was a problem hiding this comment.
You could templatize the binary operator so you can share code between And and Or.
| public: | ||
| EvaluatorType type() const { return Not; } | ||
|
|
||
| virtual void setLeft(std::unique_ptr<Evaluator> && v) { operand_ = std::move(v); } |
| public: | ||
| EvaluatorType type() const { return And; } | ||
|
|
||
| virtual void setLeft(std::unique_ptr<Evaluator> && v) { left_ = std::move(v); } |
There was a problem hiding this comment.
override on both of these
| } | ||
|
|
||
| private: | ||
| std::unique_ptr<Evaluator> left_; |
|
+1 The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
What is the difference wrt the |
|
@Martin-Grunewald |
|
Ah, OK. What happens if one instance is on path A asking for path B and one instance is on path B asking for path A - how is the deadlock resolved? Or A->B->C->D->A? |
The framework will throw an 'unrunnable schedule' exception before it starts to process data. |
|
I suppose the same throw if an instance asks for a non-existing path, right? |
|
I assume under that case you'd get a 'product missing' exception at run time. The 'unrunnable schedule' is something the framework would throw anytime two modules say they depend on each other. The PathStatus object is just threated like any other data product in the Event. |
|
Yes a Product Not Found exception is what you would get with the current version of this (although I could probably change that if we wanted some other error) |
|
OK, thanks for the clarifications! |
|
A few more differences. This new filter does not support prescalers. It does not support reading path results from prior processes, only the current one. It does not get L1 trigger results. And there are some other options in TriggerResultsFilter that it does not support. The new filter does not support the XOR operator. (These things could be added, but they are not in the current version of the new filter and as far as I know we do not plan to add them) One other comment I will add. I could be completely wrong about this because I did not test it or spend a lot of time studying it. I likely just didn't understand the code. But in reading through it, I got the impression that TriggerResultsFilter did not get precedence order of the operators correct unless you used parentheses and that the NOT operator would not work. But most likely I just didn't understand the code because I didn't spend enough time studying it. Are there tests for those things somewhere? |
cmsbuild
added
comparison-pending
tests-pending
and removed
comparison-available
tests-approved
labels
|
Pull request #20028 was updated. @cmsbuild, @smuzaffar, @Dr15Jones can you please check and sign again. |
|
The commit I just pushed should resolve all the comments from Chris. All good suggestions and I implemented them all. |
|
please test |
|
The tests are being triggered in jenkins. |
|
+1 |
cmsbuild
added
core-approved
fully-signed
and removed
core-pending
pending-signatures
labels
|
This pull request is fully signed and it will be integrated in one of the next master IBs after it passes the integration tests. This pull request will now be reviewed by the release team before it's merged. @davidlange6, @smuzaffar (and backports should be raised in the release meeting by the corresponding L2) |
|
+1 The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
+1 |
No description provided.