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

Added Breit-Wigner distribution #6419

Closed
wants to merge 5 commits into from
Closed

Added Breit-Wigner distribution #6419

wants to merge 5 commits into from

Conversation

andrewfowlie
Copy link
Contributor

I've added the relativistic Breit-Wigner distribution to scipy.stats. The _pdf and _cdf are analytic; other attributes are numerical and provided by the inherited rv_continuous class.

As for the other distributions, I define a breit_wigner_gen class, and then a breit_wigner instance of that class, which should be imported by a user.

@andrewfowlie
Copy link
Contributor Author

A few issues I need to fix:

  1. ERROR: objects in scipy.stats.all but not in refguide:: breit_wigner
  2. print 'number of continuous distributions:', len(dist_continu)

Expected:

number of continuous distributions: 94

Got:

number of continuous distributions: 95
  1. All my doctests are broken (though work on my system)


_argcheck : bool
Whether shape parameters are legitimate
"""
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: There's no need to add a full-fledged docstring to a private, computational method. A simple comment would suffice IMO (and even then, the one-line code is self-documenting enough, I'd think)

@ev-br
Copy link
Member

ev-br commented Jul 27, 2016

First and foremost, my question about the use case (#6414) still stands.

On a technical level, this is a very good start!
Assuming there is a use case, this needs some work. See, e.g. #6030 about several places a distribution needs to be registered with (__init__, _distr_param.py etc).

As for doctests, use $ python runtests -s stats --refguide-check: the doctest checker differs from the standard one.

@ev-br ev-br added scipy.stats enhancement A new feature or improvement needs-decision Items that need further discussion before they are merged or closed labels Jul 27, 2016
>>> [BW.moment(n) for n in range(1, 6)]
[122.11538219739913, 15674.920254744189, inf, 244138132.80994478, 80583978992.641495]

"""
Copy link
Member

Choose a reason for hiding this comment

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

Have a look at the docstrings of other distributions, they have magic substitution keys (compare to online docs, where the keys are expanded). I probably makes sense to reuse the standard format.

Copy link
Member

Choose a reason for hiding this comment

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

The docstring also needs a See Also section with a reference to halfcauchy.

@ev-br ev-br added the needs-work Items that are pending response from the author label Jul 27, 2016
@pv pv removed the needs-work Items that are pending response from the author label Oct 31, 2016
@frederikfaye
Copy link

Any news on this? Would love this feature!

@rgommers
Copy link
Member

The unanswered question is about whether cauchy is enough. From gh-6414:

And indeed, as a function of the square of the 4-momentum, E^2, Breit-Wigner formula is almost a Cauchy distribution, the only difference being that the support of the distribution is [0, \inf], so it's a halfcauchy.

Can you sketch a use case where a distribution of E^2 is not enough and you really need a distribution in terms of E itself?

Once that's decided, this looks close to good to go.

Copy link
Member

@ev-br ev-br left a comment

Choose a reason for hiding this comment

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

a few minor comments (assuming that a use case is going to materialize).

>>> samples = BW.rvs(1000)
>>> hist_plot = plt.hist(samples, normed=True, bins=100, range=[0, 200])

>>> plt.show()
Copy link
Member

Choose a reason for hiding this comment

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

Explicit example largely duplicates the autogenerated. Suggest deduplicating.

The CDF was found by Mathematica:

pdf = k/((m^2 - mass^2)^2 + mass^4*alpha^2)
cdf = Integrate[pdf, m]
Copy link
Member

Choose a reason for hiding this comment

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

This comment belongs to the comment of _cdf, should not be in the main docstring.

We require mass > 0 and width > 0.

The width -> 0 limit is well-defined mathematically - it's proportional
to a Dirac function. In the m -> 0 limit, the pdf vanishes.
Copy link
Member

Choose a reason for hiding this comment

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

Dirac delta function


The CDF was found by Mathematica:

pdf = k/((m^2 - mass^2)^2 + mass^4*alpha^2)
Copy link
Member

Choose a reason for hiding this comment

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

Most other distribution have TeX formulas here.

arg_1 = complex(-1)**(1. / 4.) / (-1j + alpha_)**0.5 * m / mass
arg_2 = complex(-1)**(3. / 4.) / (1j + alpha_)**0.5 * m / mass

shape_ = -1j * np.arctan(arg_1) / (-1j + alpha_)**0.5 - np.arctan(arg_2) / (1j + alpha_)**0.5
Copy link
Member

Choose a reason for hiding this comment

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

I'm pretty sure this formula could be simplified further to avoid complex arithmetics.

@frederikfaye
Copy link

The unanswered question is about whether cauchy is enough.

I think it is, see #6414 (comment).

@andrewfowlie
Copy link
Contributor Author

I don't have time for this, sorry, but would happy to see it make it into scipy :)

@rgommers rgommers deleted the branch scipy:master January 3, 2022 16:32
@rgommers rgommers closed this Jan 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A new feature or improvement needs-decision Items that need further discussion before they are merged or closed scipy.stats
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants