testdockerwcsim / XTriggerApplication

Other
2 stars 3 forks source link

Stopwatch #38

Closed tdealtry closed 4 years ago

tdealtry commented 4 years ago

Added a new class that calculates min/max/average/total time (and a histogram) for a specific function to use. The idea is that it'd be implemented in most tools, and turned on by the user in the config file. I've implemented it for the template and nhits tools so far, for the Initialise(), Execute(), Finalise() methods

Some questions/comments

brichards64 commented 4 years ago

Hi Tom, can you fix the issues i commented and resolve the conflicts please then ill merge it

tdealtry commented 4 years ago

This is ready to go, as long as you're happy with the name of the new "Utilites" directory

brichards64 commented 4 years ago

Yeh i'm not sure about utilities....

Whilst i understand it's purpose and its a good idea, it breaks wider compatibility. All of ToolDAq basically has two areas. Tools which are individual to a tool and Datamodel that is common to all tools. So normally it would go in DataModel as is. Whilst Datamodel might be a crap name as it starts to include things that are just common and not just data objects, in all the other deployments of tooldaq utilities have either just been a class in the data model folder or a utilities.h file in the data model that contained lots of useful functions.

Im not sure we want to diverge it so much as we break compatipility with other deployments and i like the simplicity of one location for all common things.

brichards64 commented 4 years ago

See here in the latest version of the application template https://github.com/ToolDAQ/ToolDAQApplication/tree/Application/DataModel

that's the route i went down

brichards64 commented 4 years ago

iv also used algorithms.h or functions.h in datamodel in the past

tdealtry commented 4 years ago

Ok, that makes sense since there's 10s of tooldaq repos. I've reverted the Utilities folder commit I think this should't live in Utilities.h, since it's a separate class (rather than a function). But could rename it UtilStopwatch or similar?

brichards64 commented 4 years ago

yeh that's a good idea, i like having a prefix that shows its a utility.

name space might work as well, util::stopwatch. Although if we do that we might loose some people who are not familiar with how to use namespaces.

what do you think?

tdealtry commented 4 years ago

I've done both, however the class util::Stopwatch is in the file UtilStopwatch.h, so maybe that's a bit confusing? Just use namespaces do you think?

brichards64 commented 4 years ago

hmmmmm...... yeh its just if people know how to use namespaces or not. but i think i prefer it as you suggested.

so please change it back to Stopwatch.h

tdealtry commented 4 years ago

Should be ready to go now

tdealtry commented 4 years ago

Back to you to check Ben - I've reduced the code required in the tools. It's not quite as simple as you maybe wanted, but it is a one-liner (If you know how to call a tools Log method from Stopwatch, we can simplify it more, but that sounds messy... or we could tell stopwatch about DataModel, but that again is a confusing dependency...)

brichards64 commented 4 years ago

other than the above suggestion looks good. There is a way of doing it that wouldn't be too messy using function pointers and alike, but this is more verbose and easy for people to understand so i think its fine as is.

brichards64 commented 4 years ago

never mind ignore the above i see you ahve the toolname in the constructor. So you had thought of that already

looks good to me then