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

Update mlab.py #3027

Closed
wants to merge 2 commits into from
Closed

Update mlab.py #3027

wants to merge 2 commits into from

Conversation

palob
Copy link

@palob palob commented Apr 30, 2014

Bugfix preventing TypeError: Added missing dtype= inside numpy.arange calls of levypdf function.
See Issue #3026 .

Bugfix preventing TypeError: Added missing **dtype=** inside numpy.arange calls of **levypdf** function.
@efiring
Copy link
Member

efiring commented Apr 30, 2014

While I agree that the missing dtype kwarg needs to be added, this looks like a classic case of a fossil that needs to be deprecated and removed--or it needs to be cleaned up, and provided with a justification for living in matplotlib. At the very least, it looks like the ValueError message has a typo; I assume it should be "even" not "event". Going just a little farther, the docstring is sadly lacking--what are the second and third arguments? If x needs to be even, that should be stated in the docstring. And, as a modernization, where N/2 is needed as an integer, it should be changed to N//2.

I have never encountered the Levy pdf before this, so I looked it up in Wikipedia; the formula given there looks nothing like the implementation here.

The PR is worth merging as-is, since it fixes a genuine bug, but if you are willing to implement any of the suggestions above, @palob, that would be even better.

Added docstring to normpdf, fragmental docstring to levypdf.
levypdf: Fixed typo in ValueError Description, introduced int division.
@palob
Copy link
Author

palob commented May 1, 2014

@efiring Done. Please note, however, that I am by no means an expert in this field. I just ran into the bug looking for however different distributions in order to set up synthetic datasets.
With that said I can't eitherreenact the correctness of the levypdf implementation for the time being.

@palob
Copy link
Author

palob commented May 1, 2014

After a little tinkering I can say this is probably not a correct implementation at least of which seems to be widely known as "Lévy distribution". It yields symmetric distributions.

@WeatherGod
Copy link
Member

Perhaps we can get confirmation from someone over in statsmodels? @wesm ,
@jseabold , @rgommers ?

On Thu, May 1, 2014 at 11:31 AM, palob notifications@github.com wrote:

After a little tinkering I can say this is probably not a correct
implementation at least of which seems to be widely known as "Lévy
distribution". It yields symmetric distributions.


Reply to this email directly or view it on GitHubhttps://github.com//pull/3027#issuecomment-41920763
.

@efiring
Copy link
Member

efiring commented May 1, 2014

@WeatherGod, @palob, now I am thinking I gave the wrong advice earlier. If this function has been completely broken--raising an exception--for a long time, and if it doesn't belong in mpl in any case, then the thing to do is to remove it now, without further ado. It can just quietly disappear, and no one will miss it.

@tacaswell
Copy link
Member

I am in agreement with @efiring, this function should just go away.

It looks like the last time this function was touched in a meaningful way (that is not style or bulk renaming of npy -> np) was when it was added (along with all of mlab.py) in e34a333 almost a decade ago.

I tried to sort out if/when the numpy api for arange changed and have a suspicion that this is a hold over from numerics

@rgommers
Copy link

rgommers commented May 3, 2014

@WeatherGod indeed doesn't look like a standard form of the Levy distribution. Should indeed go away. scipy.stats.levy should be correct and has much more functionality.

The same goes for entropy and normpdf I think, kind of random to put those in mlab.

@tacaswell
Copy link
Member

Closing this in favor of just removing the function.

@tacaswell tacaswell closed this May 3, 2014
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.

5 participants