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

Add relativistic Breit-Wigner PDF with analytic CDF #19

Merged
merged 13 commits into from
Sep 1, 2021

Conversation

simonthor
Copy link
Contributor

@simonthor simonthor commented Aug 30, 2021

Proposed Changes

This is similar to #2 except:

  • More tests
  • Analytic CDF instead of numeric
  • no merge conflicts
  • No squared function

Tests added

  • Test that the sample function works
  • Compare a point on the PDF with an already known value
  • Compare the analytical integral with the numerical integral

Checklist

  • change approved
  • implementation finished
  • correct namespace imported
  • tests added
  • CHANGELOG updated

@simonthor simonthor changed the title WIP: Add relativistic Breit-Wigner PDF with analytic CDF Add relativistic Breit-Wigner PDF with analytic CDF Aug 30, 2021
@simonthor
Copy link
Contributor Author

This PDF should now work. I was a bit unsure about a few things when implementing this though:

  • Right now, the input parameters are called m and Gamma (with a capital G), based on wikipedia's names. Should I change these names to something else?
  • Should the PDF be moved to unstable first, instead of being in the model directory?
  • Are there some other tests that should be added?
  • Right now, I do not have many type hints, as I was not sure which kinds of inputs the functions should accept. Should these type hints be added or can it be left as it is?

@jonas-eschle
Copy link
Contributor

Right now, the input parameters are called m and Gamma (with a capital G), based on wikipedia's names. Should I change these names to something else?

Let's use the non-capital one (as it's a Python convention for all names)

Should the PDF be moved to unstable first, instead of being in the model directory?

That's fine, we can go directly to production

  • Are there some other tests that should be added?

This looks fine so far

  • Right now, I do not have many type hints, as I was not sure which kinds of inputs the functions should accept. Should these type hints be added or can it be left as it is?

They can be left aside for the time being, as I am currently anyway reworking them

@zfit zfit deleted a comment from allcontributors bot Sep 1, 2021
@zfit zfit deleted a comment from allcontributors bot Sep 1, 2021
@jonas-eschle
Copy link
Contributor

@all-contributors please add @simonthor for code and ideas

@allcontributors
Copy link
Contributor

@jonas-eschle

I've put up a pull request to add @simonthor! 🎉

@zfit zfit deleted a comment from allcontributors bot Sep 1, 2021
@zfit zfit deleted a comment from allcontributors bot Sep 1, 2021
@simonthor
Copy link
Contributor Author

I should now have fixed all the issues in the code. For the record, I tried to find an analytical inverse function for the CDF using MATLAB (was not sure how to do it using sympy) but it could not find a solution.

Are there any other things that should be fixed in the code? Otherwise, I think the code is ready to be merged :)

@jonas-eschle
Copy link
Contributor

Hey looks good! I just made a small change, so make sure to pull first.

One last thing: can you factor out the computation of the PDF? Basically create a pure function called relativistic_breit_wigner_func (or something like that) which takes as arguments x, m, gamma?

@simonthor
Copy link
Contributor Author

One last thing: can you factor out the computation of the PDF? Basically create a pure function called relativistic_breit_wigner_func (or something like that) which takes as arguments x, m, gamma?

Fixed it now! Named it relbw_pdf_func since the stand-alone CDF function is called relbw_cdf_func. But can of course change the name if your function name is preferred.

@jonas-eschle
Copy link
Contributor

Good, many thanks for this! LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants