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

refactor: Make Detectors constructible without boost po #923

Merged
merged 15 commits into from
Aug 24, 2021

Conversation

paulgessinger
Copy link
Member

@paulgessinger paulgessinger commented Aug 4, 2021

Adds an overload to the detector's finalize() which accepts a newly
added Config object. The previous overload that takes boost program
options constructs such a Config object and forwards to the other
overload. This allows construction of the detector directly, with out
program options.

Additionally, the defaults for configuration values are harmonized, the
canonical source is now the Config objects, but I tried to have the po
versions consistents.

Currently this only ports

  • AlignedDetector
  • GenericDetector
  • DD4hepDetector

I think TGeoDetector should definitely be added.
@asalzburger do you think I anything else is important?

Adds an overload to the detector's `finalize()` which accepts a newly
added `Config` object. The previous overload that takes boost program
options constructs such a `Config` object and forwards to the other
overload. This allows construction of the detector directly, with out
program options.

Additionally, the defaults for configuration values are harmonized, the
canonical source is now the `Config` objects, but I tried to have the po
versions consistents.

Currently this only ports
 - `AlignedDetector`
 - `GenericDetector`
 - `DD4hepDetector`

I think `TGeoDetector` should definitely be added.
@asalzburger do you think I missed anything?
@paulgessinger paulgessinger added this to the next milestone Aug 4, 2021
@codecov
Copy link

codecov bot commented Aug 4, 2021

Codecov Report

Merging #923 (4631969) into main (6521c13) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #923   +/-   ##
=======================================
  Coverage   48.63%   48.63%           
=======================================
  Files         334      334           
  Lines       17149    17149           
  Branches     8086     8086           
=======================================
  Hits         8340     8340           
  Misses       3091     3091           
  Partials     5718     5718           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6521c13...4631969. Read the comment docs.

@asalzburger
Copy link
Contributor

Hi @paulgessinger - TGeoDetector is unavoidable as it is heavily used, but then we can leave it.

@paulgessinger
Copy link
Member Author

I'll add the TGeo detector tomorrow.

@paulgessinger
Copy link
Member Author

I will add the TGeoDetector.

@paulgessinger paulgessinger added the 🚧 WIP Work-in-progress label Aug 17, 2021
@paulgessinger paulgessinger removed the 🚧 WIP Work-in-progress label Aug 19, 2021
@paulgessinger
Copy link
Member Author

Ok. TGeoDetector added. This also rationalizes the way the various TGeo helper functions were spread around the code. They should now all be located in the TGeoDetector files.

I briefly checked with help from @niermann999 that the ITk TGeo input seems to produce the same number of layers as was the case before the change, and from checking a few obj files and test propagation, the geometry seems to be ok.

image

@paulgessinger
Copy link
Member Author

@asalzburger could you have another look and potentially reapprove?

@asalzburger asalzburger merged commit f159cce into acts-project:main Aug 24, 2021
@paulgessinger paulgessinger deleted the py/detectors branch August 24, 2021 09:33
@paulgessinger paulgessinger removed this from the next milestone Aug 26, 2021
@paulgessinger paulgessinger added this to the v12.0.0 milestone Aug 26, 2021
niermann999 added a commit to niermann999/acts that referenced this pull request Sep 2, 2021
…roject#884)

The options handling of the TGeoDetector is moved from boost to json
based configuration. It builds upon the recent reordering of the
detector options in acts-project#923. New functionality is introduces in
Examples/Io/Json. Since it needs information from the TGeo plugin,
the latter is only linked in the example io when the plugin is required.
asalzburger pushed a commit that referenced this pull request Sep 16, 2021
* Basic json config for tgeodetector and cylinder/disk splitter (#884)

The options handling of the TGeoDetector is moved from boost to json
based configuration. It builds upon the recent reordering of the
detector options in #923. New functionality is introduces in
Examples/Io/Json. Since it needs information from the TGeo plugin,
the latter is only linked in the example io when the plugin is required.
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