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: change rMax invalid_argument to warning #1227

Merged
merged 7 commits into from
Apr 13, 2022

Conversation

LuisFelipeCoelho
Copy link
Member

This PR allows for rMax in the grid to be different than the one used in BinnedSPGroup. Instead of throwing std::invalid_argument it just dumps a warning.

This is useful for ITk strip SPs. But in case we don't want to change the invalid_argument check, another option would be to add another parameter in the calculation of numRBins = (config.rMax + config.beamPos.norm()) that allows for an increase or decrease of this value.

@noemina @robertlangenberg @paulgessinger

@LuisFelipeCoelho LuisFelipeCoelho added Improvement Changes to an existing feature Impact - Minor Nuissance bug and/or affects only a single module labels Apr 6, 2022
@LuisFelipeCoelho LuisFelipeCoelho added this to the next milestone Apr 6, 2022
@codecov
Copy link

codecov bot commented Apr 6, 2022

Codecov Report

Merging #1227 (277321d) into main (2f5f06a) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1227   +/-   ##
=======================================
  Coverage   47.94%   47.94%           
=======================================
  Files         373      373           
  Lines       19495    19495           
  Branches     9152     9152           
=======================================
  Hits         9347     9347           
  Misses       3817     3817           
  Partials     6331     6331           

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@LuisFelipeCoelho LuisFelipeCoelho added the Component - Examples Affects the Examples module label Apr 6, 2022
Copy link
Contributor

@robertlangenberg robertlangenberg left a comment

Choose a reason for hiding this comment

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

Sorry, I rescind the approval, I think a "correct" configuration shouldn't produce a warning, your suggestion to add the beamSpot.norm() to the rBins is a much nicer approach.

@robertlangenberg
Copy link
Contributor

Do the ITK strips only require to increase the rBins by the beamSpot.norm? (since you also talked about reducing the number of rBins)

@LuisFelipeCoelho
Copy link
Member Author

@robertlangenberg the beamSpot.norm() is already there, I thought I could add one more parameter besides beamSpot.norm(): numRBins = (config.rMax + config.beamPos.norm() + config.newParameter)

Yes, the ITK strips only require an increase to the rBins.

@kodiakhq kodiakhq bot merged commit db7f497 into acts-project:main Apr 13, 2022
@LuisFelipeCoelho LuisFelipeCoelho deleted the different-rMax-warning branch April 13, 2022 13:30
@paulgessinger paulgessinger modified the milestones: next, v19.0.0 May 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Component - Examples Affects the Examples module Impact - Minor Nuissance bug and/or affects only a single module Improvement Changes to an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants