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

feat: Optionally decorate geometry dumps to json. #1732

Conversation

goetzgaycken
Copy link
Contributor

Added the possibility to pass a decorator to the json converter to add extra attributes to the json dump of certain objects. Decorator interfaces are defined for adding attributes to surface and volume or their material dumps.

One use case is to extend the json dumps of the ATLAS tracking geometry with the ATLAS detector element IDs.

The PR changes interfaces in Plugins/Json.

@goetzgaycken goetzgaycken force-pushed the main_optional_json_geometry_dump_decorations branch 4 times, most recently from 5b17c81 to 6fbb368 Compare December 12, 2022 22:16
@stale
Copy link

stale bot commented Jan 16, 2023

This issue/PR has been automatically marked as stale because it has not had recent activity. The stale label will be removed if any interaction occurs.

@stale stale bot added the Stale label Jan 16, 2023
@stale stale bot removed the Stale label Jan 16, 2023
@paulgessinger paulgessinger marked this pull request as ready for review January 16, 2023 09:32
@paulgessinger paulgessinger added this to the next milestone Jan 16, 2023
@codecov
Copy link

codecov bot commented Jan 16, 2023

Codecov Report

Merging #1732 (dc46faa) into main (f44f1da) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1732   +/-   ##
=======================================
  Coverage   49.54%   49.54%           
=======================================
  Files         407      407           
  Lines       22618    22618           
  Branches    10318    10318           
=======================================
  Hits        11207    11207           
  Misses       4230     4230           
  Partials     7181     7181           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@niermann999 niermann999 left a comment

Choose a reason for hiding this comment

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

Looks good in general

@goetzgaycken goetzgaycken force-pushed the main_optional_json_geometry_dump_decorations branch from 8352a7a to d318bc2 Compare January 17, 2023 08:16
@goetzgaycken
Copy link
Contributor Author

goetzgaycken commented Jan 17, 2023

changes from 8352a7a to d318bc2 :

  • rebase to main of 2023-01-17 8:05
  • explicit nullptr comparison for get<0>(src)
  • use std::false_type instead of "reinventing" the construct
  • updated copyright message of changed files to -2023

@goetzgaycken goetzgaycken force-pushed the main_optional_json_geometry_dump_decorations branch from d318bc2 to e3ca98d Compare January 17, 2023 10:20
@goetzgaycken
Copy link
Contributor Author

Changes from d318bc2 to e3ca98d

  • Fix compiler warning concerning potentially unused argument (if constrexpr(...) might disable some code)
  • Fix doxygen warning about undocumented parameter

@github-actions
Copy link

github-actions bot commented Jan 17, 2023

📊 Physics performance monitoring for dc46faa

Full report
Seeding: seeded, truth estimated, orthogonal
CKF: seeded, truth smeared, truth estimated, orthogonal
IVF: seeded, truth smeared, truth estimated, orthogonal
Ambiguity resolution: seeded, orthogonal
Truth tracking
Truth tracking (GSF)

Vertexing

Vertexing vs. mu
IVF seeded

IVF truth_smeared

IVF truth_estimated

IVF orthogonal

Seeding

Seeding seeded

Seeding truth_estimated

Seeding orthogonal

CKF

CKF seeded

CKF truth_smeared

CKF truth_estimated

CKF orthogonal

Ambiguity resolution

seeded

Truth tracking (Kalman Filter)

Truth tracking

Truth tracking (GSF)

Truth tracking

@goetzgaycken goetzgaycken force-pushed the main_optional_json_geometry_dump_decorations branch from e3ca98d to 1bc9e7a Compare January 17, 2023 13:34
@goetzgaycken
Copy link
Contributor Author

changes from e3ca98d to 1bc9e7a:

  • implement clang-format differences

@goetzgaycken goetzgaycken force-pushed the main_optional_json_geometry_dump_decorations branch from 1bc9e7a to c007e1d Compare January 17, 2023 14:16
@goetzgaycken
Copy link
Contributor Author

changes from 1bc9e7a to c007e1d :

@goetzgaycken
Copy link
Contributor Author

changes from 1bc9e7a to c007e1d :

  • removed remnant from an early idea.

@goetzgaycken goetzgaycken force-pushed the main_optional_json_geometry_dump_decorations branch from c007e1d to 4b72276 Compare January 17, 2023 15:18
Added the possibility to pass a decorator to the json converter
to add extra attributes to the json dump of certain objects.
Decorator interfaces are defined for adding attributes to surface
and volume or their material dumps.
@goetzgaycken goetzgaycken force-pushed the main_optional_json_geometry_dump_decorations branch from 4b72276 to b2bcd7a Compare January 17, 2023 17:53
@goetzgaycken
Copy link
Contributor Author

changes from 4b72276 to b2bcd7a :

  • removed unused forward declarations.

@goetzgaycken
Copy link
Contributor Author

concerning the test coverage:

  • there are some new lines which cannot be tested by a unit test because they will provoke compilation failures. Not sure how the lines are counted this would be something between 2 and 18 lines.
  • then there are some input adapters (between 12 and 34 lines), which could be tested by writing a unit test for the tracking geometry to json conversion. The latter would however require a dummy tracking geometry. The resulting "unit" test would be more an integration test than a unit test.
  • alternatively I could just call the adapters, but that would not test much.

@paulgessinger
Copy link
Member

Seems to me like the coverage decrease is somehow still from the recent change to KD-tree, maybe we should override the coverage here.

@goetzgaycken
Copy link
Contributor Author

Seems to me like the coverage decrease is somehow still from the recent change to KD-tree, maybe we should override the coverage here.

can I ran the coverage analysis locally ?

@paulgessinger
Copy link
Member

Seems to me like the coverage decrease is somehow still from the recent change to KD-tree, maybe we should override the coverage here.

can I ran the coverage analysis locally ?

This full analysis suite run by codecov, no. You can run the coverage itself locally (I think cmake --build --target coverage) which gives you coverage output that can be processed and visualized separately. I haven't run this locally in quite a while either.

@benjaminhuth benjaminhuth added Component - Examples Affects the Examples module Component - Plugins Affects one or more Plugins automerge labels Jan 18, 2023
@kodiakhq kodiakhq bot merged commit 2442fa1 into acts-project:main Jan 18, 2023
@paulgessinger paulgessinger modified the milestones: next, v23.0.0 Jan 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component - Examples Affects the Examples module Component - Plugins Affects one or more Plugins
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants