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

Retool TriggerGenericMaker to behave differently for Set<T> input/output #34

Merged
merged 6 commits into from
Jun 1, 2021

Conversation

BenLand100
Copy link
Collaborator

@BenLand100 BenLand100 commented May 27, 2021

Introduces a helper class TriggerGenericWorker to deal with the partial specialization of the do_work method. This allows queues of type Set<A> -> OUT and Set<A> -> Set<B> to be handled correctly. Unfortunately C++ does not yet allow partial specialization of methods in a template class, and the helper class is the cleanest workaround for this.

Introduces a helper class Worker to deal with the partial specialization
of the do_work method. This allows Set<A> -> OUT and Set<A> -> Set<B> to
be handled correctly
Copy link
Collaborator

@philiprodrigues philiprodrigues left a comment

Choose a reason for hiding this comment

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

There's quite a bit of nearly-the-same code here, which makes me wonder if it's worth rethinking the lack of a TCSet. If we had a TCSet, then you wouldn't need the Worker<Set<A>, OUT, MAKER> specialization (is that correct?). We could then either modify the downstream code to accept TCSets or we could add some small bit of code that takes the TCSets and "explodes" them into their component TCs for consumers. Would that be an overall improvement?

@BenLand100
Copy link
Collaborator Author

That's right, if everything were a Set<T> one of the specializations could be dropped. That said, the 'extra code' in that specialization would just get moved to whatever module does the exploding, and introduce one more queue's worth of overhead, so having this additional specialization might be the cleanest way in the end.

I did shuffle some code around to reduce how much redundant parts there were in the (renamed) TriggerGenericWorker so might be worth another look.

@philiprodrigues
Copy link
Collaborator

Yes, that looks better, and shows how we can factor out common parts between the specializations. Thanks. A couple more individual-line comments coming...

// some fraction of the times that we check, so we just continue on and try again
continue;
m_parent.m_maker->operator()(in, out_vec);
} catch (...) { // TODO BJL May 28-2021 can we restrict the possible exceptions triggeralgs might raise?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good idea. I've opened an issue in triggeralgs for tracking/discussion: DUNE-DAQ/triggeralgs#12

Logic here assumes that all Set<T> in a time slice have the same
start_time and end_time and come in on the queues in time order. All
objects of the Set<T> in the time slice are then sorted by time_start
before being passed to the operator() of algorithms.
Copy link
Collaborator

@philiprodrigues philiprodrigues left a comment

Choose a reason for hiding this comment

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

This looks good to me. Thanks for bearing with me on the iterations

@philiprodrigues philiprodrigues merged commit 64b05e9 into develop Jun 1, 2021
@philiprodrigues philiprodrigues deleted the benland100/alg-plugins branch July 1, 2021 12:30
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

Successfully merging this pull request may close these issues.

None yet

2 participants