ComPWA / ComPWA-legacy

[deprecated] C++ back-end for the Common Partial Wave Analysis framework
https://compwa.github.io/legacy
Other
9 stars 9 forks source link

FormFactorDecorator put a duplicate formfactor to a RelativisticBreitWigner #179

Closed zhang-jingqing closed 5 years ago

zhang-jingqing commented 5 years ago

The application of FormFactorDecorator class will put a duplicate formfactor to the numerator of a RelativisticBreitWigner. The RelativisticBreitWigner alreay has a formfactor in numerator, so this kind of FormFactorDecorator is not necessary and should not be applied.

The solution is dropped the application of FormFactorDecorator, and implement a formfactor for NonResonant class (i.e., put a formfactor in the numerator for production formfactor)

zhang-jingqing commented 5 years ago

I think it is better to check the relativistic breit-wigner formula currently used and implemented, before do it. Maybe the problem and solution are not exactly as above description.

spflueger commented 5 years ago

Hmm, so you think the solution above is not good? In principle the decorator gives a lot of flexibility, but you would need to rewrite a lot more code to be more granular and for the decorator pattern to work correctly. So the upper solution would require the least work to handle the cases we have now.

zhang-jingqing commented 5 years ago

I mean the RelativisticBreit has something similar to a form factor in numerator, but it is a constant and is different what decorator class put in the relativistic breit-wigner

zhang-jingqing commented 5 years ago

It seems in RelativisticBreitWigner, it has 1/ffR in numerator, where ffR is formfactor at \sqrt{s} = mR^2 FormFactorDecorator class will put a ff in the numerator, where ff is the formfactor at \sqrt{s}. ffR is a constant, while ff is not. To me, it seems we still need a ff in the numerator of relativistic breit, no matter put it in relativistic breit-wgienr directly or use the FormFactorDecorator.

spflueger commented 5 years ago

Ok, so there are two proposed solutions (I roughly summarized them and added pros and cons):

  1. The FormFactorDecorator class remains and could be renamed to ProductionFormFactorDecorator, which would help the user understand what happens. For convenience the IntensityBuilderXML should initialize the BW or other dynamical function that intrinsically use a form factor. So that by default the same form factor is used in the production and the decay width. Pros: dynamically adds form factors to amplitudes (dynamic functions do not necessarily have to work with a form factor) Cons: form factors for production and internally in the dynamic function (decay width) have to be initialized separately
  2. The FormFactorDecorator class is removed and the dynamical functions handle the form factors for themselves. However to avoid code duplication, a FormFactor class (inherits from AbstractDynamicalFunction) is created and is passed to the dynamical functions. The dynamical functions then simply evaluate the FormFactor and inserts them in the correct places. To add form factor functionality to the production process, the NonResonant dynamical function also needs to get a FormFactor instance. Pros: the FormFactor class is the single source when working with form factors Cons: every dynamical function which needs a form factor, has to take care of this itself; a form factor cannot be optional or dynamically added to an amplitude. It is just set to 1 if its not used (not sure this is the intend)

Actually the decay width for the Breit Wigner currently only takes into account a single channel. I'm not sure if it should support more channels. If this is the case, then the requirement to set the decay width and production form factors separately might be something needed?

weidenka commented 5 years ago

I certainly prefer option2. Since the form factor is containted in the energy-dependend width it is not possible to change it using the decorator pattern.

spflueger commented 5 years ago

Hmm, I'm a bit torn. I agree that the decorator pattern does not seem ideal for that case. But its a bit unfair to make this comparison, since the BW class could have been split a bit further into a decay width amplitude, which could be decorated. Also if you rename the decorator to production decorator, then its more clear that only the nominator would be modified. The xml configuration file could have the option to use different form factors for the nominator and denominator (don't know if that would ever be needed, but its actually not a bad thing to have such a feature). Then you would just initialize the production form factor and the decay width form factor with the same type. Maybe @zhang-jingqing can also state his opinion.

Independent on which option we choose, we should change the if else statements to selected the formfactor, into the strategy pattern (https://en.wikipedia.org/wiki/Strategy_pattern). Then it would also be easier for a user to add his custom form factor.

weidenka commented 5 years ago

I think we should avoid the possibility to change the form factors in the numerator and denominator seperately. This is a source of errors and is not nessessary.

zhang-jingqing commented 5 years ago

I think a usage of decorator for production form factor will avoid repeat it in different type of BW, e.g., Relativisitic BW and flatte. If use same parameter (Formfactor type and MesonRadius), then both production part and decay part of the form factor will change accordingly when the parameter changed.

weidenka commented 5 years ago

Think about an BW which is not decorated with a production form factor. It still has a form factor in the energy-depended width - which does not make sense to my option. The current implementation does not have a production form factor because it was not part of the equation given in the PDG ("Resonances Eq.49.23"). If we not decide we want to have a form factor in the numerator we should implement Eq.49.15 (I still do not understand the difference between both equations). My point here is we should implement something that we can properly reference. If someone want to the a different BW parametrization a new implementation of AbstractDynamical should be created.

Technically, I would suggest to add a member variable std::shared_ptr<FormFactor> formFactor to AbstractDynamicalFunction and define different types of form factors by implementations of the base class FormFactor. Then, different types of form factors can be used w/o re-implementing the whole BW/Flatte.

zhang-jingqing commented 5 years ago

That maybe a better way.

spflueger commented 5 years ago

In my opinion this issue is fixed with pull request #204. I would close this issue. If you disagree, please feel free to reopen it. For convenience: the production form factor is now always multiplied to a dynamics function, so the dynamics function itself should not implement a production form factor.