Skip to content
This repository was archived by the owner on Oct 11, 2021. It is now read-only.

fix: officially remove top NormalizedIntensity#417

Merged
redeboer merged 4 commits intomasterfrom
normalized-intensity
Jan 6, 2021
Merged

fix: officially remove top NormalizedIntensity#417
redeboer merged 4 commits intomasterfrom
normalized-intensity

Conversation

@spflueger
Copy link
Copy Markdown
Member

Currently the expertsystem and tensorwaves are not compatible, as the highest level NormalizedIntensity is missing but is required when performing fits in tensorwaves.

To simplify things I propose the following:
We officially remove this highest level NormalizedIntensity as this is only required by fitting using a log likelihood estimator and belong there. This has several advantages:

  1. It makes the amplitude model simpler as the normalization is not needed for the intensity definition
  2. Data generation is fast as the normalization is skipped
  3. The construction of the intensity in tensorwaves becomes more transparent
    builder = IntensityBuilder(part_list, kinematics) # note: the phase space sample here is optional
    intensity = builder.create_intensity(model)
    
    data_sample = generate_data(10000, kinematics, intensity)
    dataset = kinematics.convert(data_sample)
    
    phsp_sample = generate_phsp(100000, kinematics, random_generator, bunch_size=100)
    phspset = kinematics.convert(phsp_sample)
    
    estimator = UnbinnedNLL(intensity, dataset, phspset) # phspset is required here for the normalization

@redeboer @Leongrim @sebastianJaeger What do you think?

@spflueger spflueger added Bug Something isn't working 💡 Feature New feature added to the package labels Jan 6, 2021
@redeboer
Copy link
Copy Markdown
Member

redeboer commented Jan 6, 2021

Just a small note I can't put in the review: can __prepend_strength be removed?

def __prepend_strength(
self, intensity: IntensityNode
) -> StrengthIntensity:
strength = self.__register_parameter(
name="strength_incoherent",
value=1.0,
fix=True,
)
return StrengthIntensity(
component="incoherent_with_strength",
strength=strength,
intensity=intensity,
)

@redeboer
Copy link
Copy Markdown
Member

redeboer commented Jan 6, 2021

But generally I agree.

  1. The construction of the intensity in tensorwaves becomes more transparent

Is that because a phase space sample is not required anymore?

@redeboer
Copy link
Copy Markdown
Member

redeboer commented Jan 6, 2021

I'll fix the tests now

@spflueger
Copy link
Copy Markdown
Member Author

Just a small note I can't put in the review: can __prepend_strength be removed?

def __prepend_strength(
self, intensity: IntensityNode
) -> StrengthIntensity:
strength = self.__register_parameter(
name="strength_incoherent",
value=1.0,
fix=True,
)
return StrengthIntensity(
component="incoherent_with_strength",
strength=strength,
intensity=intensity,
)

It can be removed for now. It will be needed when using for example background parts in the amplitude model. So something like

Intensity = strength * SignalIntensity + (1-strength) * BackgroundIntensity

Strenght parameter is removed form the model
@redeboer redeboer force-pushed the normalized-intensity branch from 79e59f3 to 0718a6a Compare January 6, 2021 15:38
@redeboer
Copy link
Copy Markdown
Member

redeboer commented Jan 6, 2021

#417 (comment)
Ok. Because next question I would have is whether the StrenghtIntensity and NormalizedIntensity classes are still needed at all.

@codecov
Copy link
Copy Markdown

codecov bot commented Jan 6, 2021

Codecov Report

Merging #417 (d4aaaec) into master (0cb3d37) will decrease coverage by 0.70%.
The diff coverage is 60.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #417      +/-   ##
==========================================
- Coverage   90.18%   89.48%   -0.71%     
==========================================
  Files          26       26              
  Lines        3750     3747       -3     
  Branches      925      926       +1     
==========================================
- Hits         3382     3353      -29     
- Misses        185      201      +16     
- Partials      183      193      +10     
Flag Coverage Δ
unittests 89.48% <60.00%> (-0.71%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/expertsystem/amplitude/helicity_decay.py 88.84% <60.00%> (-0.89%) ⬇️
src/expertsystem/io/_xml/_dump.py 82.24% <0.00%> (-5.61%) ⬇️
src/expertsystem/io/_yaml/_dump.py 80.17% <0.00%> (-5.18%) ⬇️
src/expertsystem/io/_yaml/_build.py 84.87% <0.00%> (-5.05%) ⬇️
src/expertsystem/io/_xml/_build.py 81.52% <0.00%> (-3.83%) ⬇️

@spflueger
Copy link
Copy Markdown
Member Author

spflueger commented Jan 6, 2021

But generally I agree.

  1. The construction of the intensity in tensorwaves becomes more transparent

Is that because a phase space sample is not required anymore?

Yes, in cases where there is no internal normalizations going on, the phase space sample is not needed anymore. However it may still be needed in the the IntensityBuilder, if normalizations within the amplitude model appear. For instance when the model includes

  • signal and a background term. Then it can be beneficial to normalize both parts to have meaningful strengths (value E [0,1])
  • normalizing amplitudes. Same logic as above. Magnitudes then correspond to fit fractions.
  • maybe more use cases

Copy link
Copy Markdown
Member

@redeboer redeboer left a comment

Choose a reason for hiding this comment

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

LGTM!
Just one comment #417 (comment) about the if else statement

@spflueger
Copy link
Copy Markdown
Member Author

#417 (comment)
Ok. Because next question I would have is whether the StrenghtIntensity and NormalizedIntensity classes are still needed at all.

See #417 (comment)

@redeboer redeboer merged commit d7f81eb into master Jan 6, 2021
@redeboer redeboer deleted the normalized-intensity branch January 6, 2021 15:56
redeboer added a commit that referenced this pull request Jan 6, 2021
Strength intensity is removed by #417
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Bug Something isn't working 💡 Feature New feature added to the package

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants