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

Split distributions #3128

Closed
wants to merge 10 commits into from
Closed

Conversation

argriffing
Copy link
Contributor

Break the distributions.py file into three files. The scipy tests all pass for me, and I guess we will find out if they pass on Travis. The docstrings are built on the fly and I do not think this docstring generation code is tested.

@argriffing
Copy link
Contributor Author

One of the Travis builds failed because pypi didn't provide mpmath in a timely manner

Downloading/unpacking mpmath
HTTP error 503 while getting https://pypi.python.org/packages/source/m/mpmath/mpmath-0.17.tar.gz#md5=e27af3a77bc39c8745d5d1c09a8247ac (from https://pypi.python.org/simple/mpmath/)

I'll check if closing the PR and re-opening will make Travis retry downloading mpmath.

@argriffing argriffing closed this Dec 6, 2013
@argriffing argriffing reopened this Dec 6, 2013
@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 83e61ea on argriffing:split-distributions into 47f10f7 on scipy:master.

@WarrenWeckesser
Copy link
Member

I haven't looked too closely at this yet, so I have just a quick initial comment: all the new files should be "private" modules (i.e. give the filenames an underscore prefix). We're trying to avoid the proliferation of public submodules, so unless there is a compelling reason, the default should be "private" for new modules.

@argriffing
Copy link
Contributor Author

I deliberately used discrete_distns and continuous_distns without underscores, with the idea that they could be imported as part of the public interface, for example if someone wanted only continuous or discrete distributions. The __all__ list of the original distributions module is now cleanly divided between them. I gave the third module an underscore prefix because it should not be part of the public interface.

But if this is not a good idea then I will give them underscores.

@rgommers
Copy link
Member

rgommers commented Dec 7, 2013

I'd rather not have more public modules. We already have stats.norm and stats.distributions.norm, which is one too many.

@josef-pkt
Copy link
Member

I'm not sure this PR works.

You need to copy paste, delete add in the same commit to preserve git history.

Without blame it will be very painful to "maintain" distributions.

@argriffing
Copy link
Contributor Author

OK, I've added underscore prefixes to the submodules.

@argriffing
Copy link
Contributor Author

I'm not sure this PR works.
You need to copy paste, delete add in the same commit to preserve git history.
Without blame it will be very painful to "maintain" distributions.

Closing, with the idea to make a new PR that is less awkwardly gitted

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 861f624 on argriffing:split-distributions into 47f10f7 on scipy:master.

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

Successfully merging this pull request may close these issues.

6 participants