LDMX-Software / ldmx-sw

The Light Dark Matter eXperiment simulation and reconstruction framework.
https://ldmx-software.github.io
GNU General Public License v3.0
22 stars 20 forks source link

Adding Trigger submodule #950

Closed therwig closed 1 year ago

therwig commented 3 years ago

I would like to include a new submodule Trigger in ldmx-sw. The submodule would include sub-directories for two types of algorithm implementations. The first is pure software, which can be compiled within ldmx-sw, directly using the standard trigger reco objects, supports floating-point calculations, etc. The second is hls code, which could be run in a standalone setup and (eventually) also directly in ldmx-sw. This repo would also be included as a submodule in the APx firmware repository.

omar-moreno commented 3 years ago

Following up on the PR:

1) Move Algo_HLS to it's own repo (TriggerHLS) and move code in Algo into the trigger submodule. Since Algo_HLS runs standalone, then it really doesn't belong in ldmx-sw. 2) Make sure the code formatting follows the Google C++ style. 3) Use namespace trigger.

I'll add more checks to the code once these updates are in. I will also plan to move existing trigger code into this submodule.

therwig commented 3 years ago

Hi Omar, thanks for the comments. We've started informally discussing how to setup and handle the HLS algorithms that we're beginning to assemble, and probably we may want to discuss in a TDAQ meeting (or another appropriate venue) at some point.

To me 2 and 3 are not controversial and I'll make the changes.

On 1, I can say a little more to motivate this choice. A main idea behind the combined repository is that HLS gives us the ability to directly run the firmware in ldmx-sw. If someone wants to develop a trigger based on ldmx-sw edm objects, they can do so (and these may live in Trigger/Algo) which will remain an effective way to do studies. But once the algorithm is implemented in HLS, it can be directly called in the same Analyzer, e.g. to make comparisons and can ultimately be used as a direct emulation of the firmware.

Because the HLS is needed in both the standalone and ldmx-sw environments, the combined repository allows us to keep all of the algorithms in one repo, where they can also directly take advantage of common utilities and helpers as necessary, duplication (e.g. the ap_int types, compression/packing functions, float-to-firmware conversion functions and FW test vector-writers).

There are different ways to achieve the goal, and I don't claim that this is the foolproof best option, but I realize that I likely did not give a sufficient explanation of the idea in the issue text above.

therwig commented 3 years ago

Looping in also @nhanvtran (related WIP PR is #952).

omar-moreno commented 3 years ago

We should discuss this in more detail at the software developers meeting. My thoughts below:

Hi Omar, thanks for the comments. We've started informally discussing how to setup and handle the HLS algorithms that we're beginning to assemble, and probably we may want to discuss in a TDAQ meeting (or another appropriate venue) at some point.

To me 2 and 3 are not controversial and I'll make the changes.

On 1, I can say a little more to motivate this choice. A main idea behind the combined repository is that HLS gives us the ability to directly run the firmware in ldmx-sw. If someone wants to develop a trigger based on ldmx-sw edm objects, they can do so (and these may live in Trigger/Algo) which will remain an effective way to do studies. But once the algorithm is implemented in HLS, it can be directly called in the same Analyzer, e.g. to make comparisons and can ultimately be used as a direct emulation of the firmware.

OK, I see the logic behind wanting to do this. However, I would like to change things up such that they conform to the ldmx-sw modules format.

I think this is enough to start and also avoids having to maintain this module separately from the rest of the ldmx-sw modules. Let me know if you have questions.

Because the HLS is needed in both the standalone and ldmx-sw environments, the combined repository allows us to keep all of the algorithms in one repo, where they can also directly take advantage of common utilities and helpers as necessary, duplication (e.g. the ap_int types, compression/packing functions, float-to-firmware conversion functions and FW test vector-writers).

There are different ways to achieve the goal, and I don't claim that this is the foolproof best option, but I realize that I likely did not give a sufficient explanation of the idea in the issue text above.

nhanvtran commented 3 years ago

Sounds good - don’t have too much to add here. I think some of the thinking behind implementation would be good to be discussed in a software meeting. When’s the next one planned for?

therwig commented 3 years ago

All files should follow the Google C++ style and namespace conventions set by ldmx-sw.

@omar-moreno, to make sure that I do this efficiently, are the LDMX-specific conventions explicitly documented somewhere (e.g. a Contributing.md), or is the best way to read all of the style guide and look at other submodules for namespace conventions?

omar-moreno commented 3 years ago

On Wed, Jan 13, 2021 at 10:13 AM Christian Herwig notifications@github.com wrote:

All files should follow the Google C++ style and namespace conventions set by ldmx-sw.

@omar-moreno https://github.com/omar-moreno, to make sure that I do this efficiently, are the LDMX-specific conventions explicitly documented somewhere (e.g. a Contributing.md), or is the best way to read all of [ https://google.github.io/styleguide/cppguide.html](the style guide) and look at other submodules for namespace conventions?

Reading the actual style guide is best although there are formatters in existence that will do this automatically. The one I have been using is: https://clang.llvm.org/docs/ClangFormat.html

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/LDMX-Software/ldmx-sw/issues/950#issuecomment-759627156, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA4JMXFVV245K6IDQSE5CLTSZXPFRANCNFSM4V6MLLPA .

therwig commented 3 years ago

Todos from last sw dev meeting: