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

Add option to switch between hierarchy and averaging constraints in MP-b... #6254

Merged
merged 1 commit into from Nov 11, 2014

Conversation

mschrode
Copy link

@mschrode mschrode commented Nov 6, 2014

...ased tracker alignment

The default behaviour is now to use averaging constraints after
problems with the fit convergence have been found when using
hierarchy constraints. This is explained in more detail in this
presentation by C. Kleinwort,

https://indico.cern.ch/event/337178/session/0/contribution/5/material/slides/0.pdf

Note that the implementation of the averaging constraints is still
approximate for the time being, but adequate for current purposes
and superior to the current version of the hierarchy constraints.

…P-based tracker alignment

The default behaviour is now to use averaging constraints after
problems with the fit convergence have been found when using
hierarchy constraints. This is explained in more detail in this
presentation by C. Kleinwort,

https://indico.cern.ch/event/337178/session/0/contribution/5/material/slides/0.pdf

Note that the implementation of the averaging constraints is still
approximate for the time being, but adequate for current purposes
and superior to the current version of the hierarchy constraints.
@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 6, 2014

A new Pull Request was created by @mschrode (Matthias Schroeder) for CMSSW_7_3_X.

Add option to switch between hierarchy and averaging constraints in MP-b...

It involves the following packages:

Alignment/CommonAlignmentAlgorithm

@diguida, @cerminar, @cmsbuild, @nclopezo, @rcastello, @mmusich can you please review it and eventually sign? Thanks.
@ghellwig, @pakhotin, @frmeier, @mmusich, @tlampen this is something you requested to watch as well.
You can sign-off by replying to this message having '+1' in the first line of your reply.
You can reject by replying to this message having '-1' in the first line of your reply.
@nclopezo, @ktf you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

@frmeier
Copy link
Contributor

frmeier commented Nov 6, 2014

+1
This solves a convergence issue discovered recently, as mentioned in the presentation linked. Supersedes some manual hot fix. As @mschrode mentioned, this is not the final word but improves the current state. Will have to backport this to other releases.

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 7, 2014

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 7, 2014

@diguida
Copy link
Contributor

diguida commented Nov 11, 2014

@mschrode @frmeier @mmusich
This PR adds an enum data member for distinguishing between the function minimised in the alignables fit for the MillePede alignment. The current default is set in the config file to be approximate_averaging as the hierarchy constraints method has a problem, introduced by #2370
The approximate average method is currently the most reliable way to run MP.

@diguida
Copy link
Contributor

diguida commented Nov 11, 2014

+1
Based on feedback by @frmeier and @mmusich

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_3_X IBs unless changes (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @davidlange6, @nclopezo, @ktf, @smuzaffar

davidlange6 added a commit that referenced this pull request Nov 11, 2014
Add option to switch between hierarchy and averaging constraints in MP-b...
@davidlange6 davidlange6 merged commit 1d3e23f into cms-sw:CMSSW_7_3_X Nov 11, 2014
@mschrode
Copy link
Author

mschrode commented Dec 3, 2014

This PR is still marked as "orp-pending", even though it is "fully merged and closed". Is this a bug of the labeler or is there some information / approval missing? Thx!

@diguida
Copy link
Contributor

diguida commented Dec 3, 2014

@mschrode it is due to the fact that David merged the PR without formally approving it (OTOH he did approve it, as he included it in release).

@mschrode
Copy link
Author

mschrode commented Dec 3, 2014

@diguida OK I see. Thx for the explanation!

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

Successfully merging this pull request may close these issues.

None yet

6 participants