Skip to content

Johnfreeman/trgdataformats issue59 review impl - #61

Draft
jcfreeman2 wants to merge 15 commits into
developfrom
johnfreeman/trgdataformats_issue59_review_impl
Draft

Johnfreeman/trgdataformats issue59 review impl#61
jcfreeman2 wants to merge 15 commits into
developfrom
johnfreeman/trgdataformats_issue59_review_impl

Conversation

@jcfreeman2

@jcfreeman2 jcfreeman2 commented Aug 6, 2026 •
edited by eflumerf
Loading

Copy link
Copy Markdown
Contributor

Description

This is a PR which engages primarily in refactoring trgdataformats as part of the implementation of its code review, discussed in #59 and part of the broader data formats review effort.

Right now (8/6/2026) this PR is in draft status. In particular, I need to deal with the README.md in include/trgdataformats. Additionally, GitHub is having problems today so I haven't had the chance to create a test build. But in terms of the changes to the source code itself, I have things basically where I want them, though some changes will be more controversial than others. These changes include:

  • Addition of more unit tests
  • A trgdataformats_binding_tests.py script which tests the Python bindings
  • Short descriptions of the purposes of the various headers
  • static_assertions which do various things. Upholding the integrity of the bit layouts of overlays / prodding developers to remember to bump the version of structs when needed / making it hard to add a new TriggerCandidateData::Type while forgetting to account for it in string_to_trigger_candidate_type and trigger_candidate_type_to_string.
  • Adding a set_inputs function to TriggerObjectOverlay. I understand if others think this overkill; originally (as I describe in the Issue) I thought I would pair it with get_inputs function but I'm not sure get_inputs has any point at all, really.
  • Moved some constants which were only used by TriggerPrimitive out of Types.hpp and into TriggerPrimitive.hpp.
  • Added some "overlay standard" functions to TriggerPrimitive - operator<, get_timestamp and set_timestamp. These are things the type adapter can use.

Some things I've observed / been thinking about beyond just the changes in this PR:

  • If we can get trgdataformats to depend on daqdataformats, we no longer need to redefine a timestamp_t type and I believe we could replace detid_t with SourceID (see Phil Rodrigues's 2021-era comments which I left in the code).
  • That the way TriggerCandidateData and TriggerActivityData is laid out, the compiler inserts a lot of padding - e.g., a TriggerActivityData instance is 80 bytes, but contains a lot less than 80 bytes worth of information. As I wanted to focus on refactoring for this PR, I didn't play around with the ordering of members, but it's something perhaps worth thinking about.
  • Where to make overlay concepts (e.g., "has a less than operator" or "has a get_timestamp() function") accessible not just to fddetdataformats but trgdataformats as well

Note also to build this you'll want to check out the branches of the same name in dfmodules, fdreadoutlibs, hdf5libs, trigger and triggeralgs; those changes are all quite minor.

Type of change

  • [X ] Documentation (non-breaking change that adds or improves the documentation)
  • New feature or enhancement (non-breaking change which adds functionality)
  • [X ] Optimization (non-breaking change that improves code/performance)
  • Bug fix (non-breaking change which fixes an issue)
  • Breaking change (whatever its nature)

Testing checklist

  • Unit tests pass (e.g. dbt-build --unittest)
  • Minimal system quicktest passes (pytest -s minimal_system_quick_test.py)
  • Full set of integration tests pass (dunedaq_integtest_bundle.sh)
  • Python tests pass if applicable (e.g. python -m pytest)
  • Pre-commit hooks run successfully if applicable (e.g. pre-commit run --all-files)

Comments here on the testing

Further checks

  • Code is commented where needed, particularly in hard-to-understand areas
  • Code style is correct (dbt-build --lint, and/or see https://dune-daq-sw.readthedocs.io/en/latest/packages/styleguide/)
  • If applicable, new tests have been added or an issue has been opened to tackle that in the future.
    (Indicate issue here: # (issue))

Your Name added 15 commits July 14, 2026 13:21
Your Name
JCF: Issue #59: update variable names to conform to Style Guide; all-…
6746ab3 
…caps is reserved for reference to #defines and macros
Your Name
JCF: Issue #59: protect against the whole-detector det_id value from …
5f4497e 
…equaling the invalid det_id value
Your Name
JCF: Issue #59: various refactors, listed below
90d351f 
* A more systematic approach to dealing with unsigned values when it
  comes to our linter, including removal of NOLINT directives when the
  actual type is unsigned but the linter wouldn't be able to determine
  this

* static_assert-ions for the developers benefit meant to remind them
  to bump the version if they add or remove variables from
  TriggerCandidateData or TriggerActivityData. Not bulletproof, but
  much better than nothing.

* static declaration of map in get_trigger_candidate_type_names so
  that it doesn't get constructed every time    the function is called

* Removal of exception throwing as flow-control mechanism in
  trigger_candidate_type_to_string; exceptions are costly and should
  generally just be used for problems which are exceptional and can't
  be handled locally.
Your Name
JCF: Issue #59: a few more refactors
88fde61 
* Make the type of the enums in the structs explicit so we can explicitly reason about their size

* Have string_to_trigger_candidate_type return a TriggerCandidateData::Type rather than an int

* Add some static_assert checks to TriggerPrimitive to confirm the integrity of its layout

* Add a TODO to Types.hpp basically pointing out that we should
  investigate making trgdataformats depend on daqdataformats
Your Name
JCF: Issue #59: analogous to what's done with the overlay classes in
9fd87b6 
fddetdataformats, add some functions which can be called directly in
the implementation of the trigger type adaptor and would allow for
templatization
Your Name
JCF: Issue #59: in general, it's better to call static_assert than st…
0a7392a 
…atic_asset
Your Name
JCF: Issue #59: add a kind of catch-all set of unit tests called "Tri…
9d6114e 
…ggerDataStructures_test"; note that the lack of a default value for detid in the code causes a failure
Your Name
JCF: Issue #59: fix the new failing unit test
c22d9b3
Your Name
JCF: Issue #59: update the Python bindings to reflect the timestamp +…
52194fb 
… comparison functions added to TriggerPrimitive as well as support for what's in Types.hpp
Your Name
JCF: Issue #59: add a couple of missing Python bindings, reorder thei…
71942e9 
…r declarations to match the header, and add a static_assert meant to alert developers to update bindings when versions are changed
Your Name
JCF: Issue #59: switch the container which connects the TriggerCandid…
1f59155 
…ateData::Type to a name from an std::map into an std::array so we can perform compile-time checks on it
Your Name
JCF: Issue #59: fix the didn't-do-what-I-wanted-it-to-do static_asser…
d4f8540 
…tion and give the sentinel enum an even more obvious name so developers won't add enums after it
Your Name
JCF: Issue #59: add Python scripts which test the Python bindings, ca…
20da661 
…lled by the top-level trgdataformats_binding_tests.py script
Your Name
JCF: Issue #59: add a set_inputs function to the overlay struct
231f679
Your Name
JCF: Issue #59: add brief overview comments at the beginning of the h…
2254600 
…eaders
eflumerf
eflumerf approved these changes Aug 7, 2026
View reviewed changes

@eflumerf eflumerf left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good, unit and integration tests pass. Had to add fdreadoutlibs to the required checkout list

@eflumerf

eflumerf commented Aug 7, 2026

Copy link
Copy Markdown
Member

Can you create and link PRs for those other repos?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@eflumerf eflumerf eflumerf approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jcfreeman2 @eflumerf