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

fix!: use es.FormFactor in _RelativisticBreitWigner #187

Merged
merged 5 commits into from
Jan 6, 2021

Conversation

redeboer
Copy link
Member

@redeboer redeboer commented Jan 5, 2021

Small fix for a bug that was forgotten to be changed in #134 (the change was correctly implemented for _NonDynamic

class _NonDynamic:
def __init__(
self,
dynamics_props: DynamicsProperties,
form_factor: Optional[es.FormFactor] = None,
) -> None:

Not sure why this wasn't spotted before. Long-term, this is better addressed through #162.

To-do list

  • Correctly import form factor from AmplitudeModel for a relativistic BW
  • Update test values for the new model (deterministic test and callback test)
  • Revisit fit example of the example workflow

@redeboer redeboer added the 🐛 Bug Something isn't working label Jan 5, 2021
@redeboer redeboer self-assigned this Jan 5, 2021
@redeboer
Copy link
Member Author

redeboer commented Jan 5, 2021

@spflueger It seems that the pre-factor for relativistic Breit-Wigners was converted incorrectly form the AmplitudeModel so far. This bug fix causes some of the tests to break again, but the model also looks completely different. Compare the models in the workflow notebooks prior to optimizing:

v0.1.3 (see here)
image

this branch (generated locally with c35f270)
image

Is this the expected behaviour when adding a form-factor?

@redeboer
Copy link
Member Author

redeboer commented Jan 6, 2021

#187 (comment) So actually there are three separate problems:

  1. the check on whether the correct formalism is used was faulty (fixed by fix: throw invalid L error correctly #188)
  2. the form factor was imported incorrectly from the es.AmplitudeModel (this PR fix!: use es.FormFactor in _RelativisticBreitWigner #187).
  3. there is a factor missing in the relativistic BW without form factor:
    return relativistic_breit_wigner(
    dataset[self._dynamics_props.inv_mass_name],
    self._dynamics_props.resonance_mass,
    self._dynamics_props.resonance_width,
    )

    should have mass and width, like here
    return relativistic_breit_wigner(
    inv_mass_squared, mass0, width
    ) * atfi.complex(mass0 * gamma0 * atfi.sqrt(ff2), atfi.const(0.0))

These three problems can be addressed by separate PRs (more transparent). This PR will need to fix some of the test values and rethink the example parameter values of the workflow notebook.

@redeboer
Copy link
Member Author

redeboer commented Jan 6, 2021

22d5eba fixes the tests, apart form test_callbacks.py. That test has an additional problem: it checks whether the parameters that the optimizer produces after the optimization is finished are equal to the 'latest' parameters stored to the traceback. However, (1) the callbacks by default store write the latest parameters every 10 steps (fixed by 59ee43a) and (2) the values are not written on the last step. So the "latest" parameters in the files that the callbacks (YAML and CSV) dump are actually not the latest parameters.

I can fix this tests quickly, but we may have to think of a fix here (@spflueger)?

The last optimize call is currently not written to the fit traceback.
Fixing this requires more work and is better handled in
#186
@codecov
Copy link

codecov bot commented Jan 6, 2021

Codecov Report

Merging #187 (db3330f) into master (574fe16) will decrease coverage by 1.42%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #187      +/-   ##
==========================================
- Coverage   83.66%   82.24%   -1.43%     
==========================================
  Files          13       13              
  Lines         704      704              
  Branches       97       97              
==========================================
- Hits          589      579      -10     
- Misses         83       90       +7     
- Partials       32       35       +3     
Flag Coverage Δ
unittests 82.24% <0.00%> (-1.43%) ⬇️

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

Impacted Files Coverage Δ
...ensorwaves/physics/helicity_formalism/amplitude.py 75.37% <0.00%> (-0.38%) ⬇️
src/tensorwaves/data/generate.py 87.03% <0.00%> (-9.26%) ⬇️
src/tensorwaves/optimizer/callbacks.py 72.16% <0.00%> (-4.13%) ⬇️

@redeboer redeboer changed the title fix: use es.FormFactor in _RelativisticBreitWigner fix!: use es.FormFactor in _RelativisticBreitWigner Jan 6, 2021
@redeboer redeboer merged commit 8b69eb8 into master Jan 6, 2021
@redeboer redeboer deleted the fix-form-factor branch January 6, 2021 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant