gcowan / RapidSim

Phase space generation of b hadron decays
MIT License
19 stars 29 forks source link

Make eval method of RapidParam more general #16

Closed particleist closed 7 years ago

particleist commented 7 years ago

In order to allow storing IP values, I modified a bit the eval method in RapidParam, so that it now takes a LorentzVector and a pair of IP,SIGMAIP. This is not really a scaleable solution, and passing it the particle would presumably mean shifting some of the logic which now lives in the fillSingleHypothesis method of RapidHistWriter. I am not sure what to propose but we probably want to make this more scalable.

dcraik commented 7 years ago

Currently there are two types of RapidParam used in the code. Parameters set up using the param keyword have a specific RapidParam object which keeps an internal list of the particles it needs to use - for these we call eval(). Parameters set up using the other keywords only have a single instance and for these we currently call eval(TLorentzVector) where we can pass in the momentum for the specific particle or combination we want - a couple of parameter types are already more complicated and cannot be used in this way.

A quick solution is to ban the IP parameter types from being used in the generic way and require specific RapidParam objects to be created by using the param keyword but this would mean extra work for the user as one could no longer add IP to paramsStable or paramsDecaying.

This also feels like quite a messy fix. It would make more sense in the long run to completely remove the generic RapidParam objects that use this version of eval and instead define a specific one for each particle or combination. I'd need to look through the code a bit more to remind myself if this is feasible.

I think it would drastically simplify fillSingleHypothesis but the logic of reading the various default parameter lists would have to go somewhere else (either in RapidConfig or RapidHistWriter).

gcowan commented 7 years ago

After a chat with @dcraik I have refactored the code such that all RapidParam objects are constructed with a vector of RapidParticles which should be used in their evaluation. This has increased the number of RapidParam objects but makes things a bit more flexible elsewhere in the code. For example, we now only have a single RapidParam::eval() method (no arguments) that can be used for both "default" parameters and those that are created by the user. Changes are in the branch rapidparam, corresponding to commit e7effa83d23c1347cda4cf7b8e25c13e4a3376d6. Can someone else give this a try and check for any issues? If none are found I will merge with the master and close this issue. We can the move onto the next phase of development. There are perhaps a few places where the code could be tidied up (i.e., places where we duplicate lines as we still have separate vectors of params for stable, decaying, 2-, 3-body etc).

gcowan commented 7 years ago

Hmm, the refactoring works, but I think there will be a problem with the particle mis-ID and filling histograms for the alternative hypotheses. Will need to think about this.

gcowan commented 7 years ago

Actually, the fix was trivial in the end. Now pushed and, I think, ready to merge.

gcowan commented 7 years ago

Merge has taken place.