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: Make binning explicitly configurable in TGeo example #1126

Merged
merged 13 commits into from
Jan 18, 2022

Conversation

niermann999
Copy link
Contributor

@niermann999 niermann999 commented Dec 17, 2021

This PR makes the surface binning in the TGeo Detector example explicitly configurable. In the json configuration a vector of bins for loc0 and loc1 is added for the negative, central and positive detector regions respectively. The binning must be set per layer, meaning that the number of layers must be known when configuring the detector building. Alternatively, a single binning entry of 0 can be used to trigger autobinning.
Also adds itk module splitter options to python bindings and enables tgeo json config dump from existing json config.

@niermann999 niermann999 added this to the WIP milestone Dec 17, 2021
@niermann999 niermann999 changed the title feat - Make binning explicitly configurable in TGeo example feat: Make binning explicitly configurable in TGeo example Dec 17, 2021
@niermann999
Copy link
Contributor Author

Some more testing is needed, but the trackstate plot looks better:

@niermann999
Copy link
Contributor Author

trackstates_r_z

@paulgessinger
Copy link
Member

Btw, I think root can actually evaluate sqrt(g_x_hit^2 + g_y_hit^2).

@niermann999
Copy link
Contributor Author

Btw, I think root can actually evaluate sqrt(g_x_hit^2 + g_y_hit^2).
Good point!

@paulgessinger paulgessinger linked an issue Dec 17, 2021 that may be closed by this pull request
@codecov
Copy link

codecov bot commented Dec 17, 2021

Codecov Report

Merging #1126 (84699c9) into main (25a0f0b) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1126   +/-   ##
=======================================
  Coverage   48.18%   48.18%           
=======================================
  Files         341      341           
  Lines       17644    17644           
  Branches     8323     8323           
=======================================
  Hits         8502     8502           
  Misses       3367     3367           
  Partials     5775     5775           

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 25a0f0b...84699c9. Read the comment docs.

@niermann999 niermann999 added 🚧 WIP Work-in-progress Bug Something isn't working Component - Examples Affects the Examples module Feature Development to integrate a new feature labels Dec 17, 2021
@niermann999 niermann999 removed the 🚧 WIP Work-in-progress label Dec 17, 2021
@niermann999 niermann999 marked this pull request as ready for review December 17, 2021 17:43
@niermann999 niermann999 marked this pull request as draft December 17, 2021 17:55
@niermann999 niermann999 marked this pull request as ready for review December 17, 2021 19:16
@niermann999 niermann999 modified the milestones: WIP, next Dec 17, 2021
@timadye
Copy link
Contributor

timadye commented Jan 12, 2022

Thanks for this update. I tested it with your ITk JSON in acts-detector-examples!11 and it works perfectly for me too - just as you show above.

With the Python bindings, we should also update the Examples/Scripts/Python/itk.py example. That does a TGeoDetector.create() with the whole ITk configuration specified in Python, rather than JSON. I think it makes sense to include it with this PR, so fix and test are consistent. At the moment itk.py produces lots of warning messages like:

11:06:01    InnerPixelsL   WARNING   Incorrect binning configuration found for loc1 protolayer #0. Either no configuration or too few configurations were provided. Using auto-binning for this layer instead.

I guess the change needed is to add binning0 and binning1 settings to the TGeoDetector.create() call in itk.py? Does it also need other changes, eg. for the module splitting?

@niermann999
Copy link
Contributor Author

That's a good point. Yes, I think it needs both configurations added. The binning option is at least already registered in this PR, I will add the configuration

@kodiakhq kodiakhq bot merged commit 7a4814b into acts-project:main Jan 18, 2022
@paulgessinger paulgessinger modified the milestones: next, v17.0.0 Mar 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Bug Something isn't working Component - Examples Affects the Examples module Feature Development to integrate a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ITk standalone geometry has unsuitable binning
3 participants