Page MenuHomeHEPForge

Implementation of n-dimensional Gaussian constraints
ClosedPublic

Authored by mwhitehe on Jun 7 2023, 10:44 AM.

Details

Summary

Implementation of n-dimensional Gaussian constraints on fit parameters
Closes T214

Test Plan

Tests with toys show spreads and correlations between parameters that are consistent with the constraints applied

Diff Detail

Repository
rLAURA laura
Branch
mwhitehe_nDconstraints
Lint
No Lint Coverage
Unit
No Test Coverage
Build Status
Buildable 251
Build 251: arc lint + arc unit

Event Timeline

I've included the example of how to use the constraint - likely not the best place to have it in the end, but for now probably useful.

mwhitehe changed the visibility from "All Users" to "Public (No Login Required)".
mwhitehe changed the edit policy from "All Users" to "Laura (Project)".
mwhitehe added a project: Laura.
  • Updates to move some functionality to LauFitObject
  • Update code after move to LauFitObject and extend to LauSimFitCoordinator
  • Add documentation and fix a bug from loop updates
tlatham requested changes to this revision.Jun 8 2023, 1:37 PM
tlatham added a reviewer: tlatham.

Many thanks Mark, this looks great. Just a few small suggestions here and there.

inc/LauFitObject.hh
37–38

Do you need TSystem.h here?
Should probably be TVectorD.h.

src/LauAbsFitModel.cc
39

Can drop this since it's already included in LauFitObject.hh

760–764

No need for the if ( ! conVars_.empty() ).
Suggest instead to replace the for loop with:
for ( LauAbsRValue* par : conVars_ )
then (*iter) becomes par

774

I wonder, can we have this TVectorD as a member of the StoreNDConstraints struct as well? It should be possible to construct it with the right size when the struct is first created in LauFitObject::addNDConstraint.

775

To avoid copying the vector

933–935

Can we call these something a bit more easily understandable?
Maybe: constraint, parname, and fitPar?

942

Better to push directly into itervec.conLauPars_ and avoid the need for the params vector.

In fact, as currently written, I think that if you had more than one n-dimensional constraint you'd end up with too many parameters in all but the first one since params is declared outside all loops and is never cleared.

src/LauFitObject.cc
31–32
98

Here you're relying on the fact that checkRepetition aborts the code.
I think it would be better to have checkRepetition return a status boolean and then we can print another error and abort here.

127

Same comment as above

151–172

I would be inclined to allow the checks to continue so that you see all errors rather than one at a time. So you could have an allOK boolean that is initially true, then if an error is encountered it gets set to false. Then at the end of the function you just return that boolean.

Perhaps also try to improve the loop variable names.

src/LauSimFitCoordinator.cc
788

Same comments on this function as for the corresponding one in LauAbsFitModel

845

Same comments on this function as for the corresponding one in LauAbsFitModel

This revision now requires changes to proceed.Jun 8 2023, 1:37 PM
  • Address review comments

Just a few trivial tidy-ups

inc/LauFitObject.hh
145
149
src/LauAbsFitModel.cc
760–764

To be picky, I would have used par singular here

770

No need for this if, the loop will anyway not execute if it's empty

938
src/LauFitObject.cc
150
175
src/LauSimFitCoordinator.cc
779–783
789

Similarly, no need for the if here

853

Please also add an entry in the release notes, which should reference either this Differential revision or the corresponding Maniphest task

mwhitehe marked 10 inline comments as done.
  • Further tweaks and added release notes, example usage removed from GenFitDpipi
tlatham retitled this revision from First draft of n-dimensional Gaussian constraints to Implementation of n-dimensional Gaussian constraints.
tlatham edited the summary of this revision. (Show Details)
tlatham edited the test plan for this revision. (Show Details)
This revision is now accepted and ready to land.Jun 9 2023, 5:39 PM