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

REF/TST: `ProbPlot` now uses `resettable_cache` and added some kwargs to plotting fxns #1179

Merged
merged 5 commits into from Dec 24, 2013

Conversation

Projects
None yet
3 participants
@phobson
Copy link
Contributor

commented Nov 10, 2013

Also reworked the tests.

@coveralls

This comment has been minimized.

Copy link

commented Nov 10, 2013

Coverage Status

Coverage remained the same when pulling a679b71 on phobson:ProbPlot-enhancements-and-tests into 9d4b1f8 on statsmodels:master.

(self.dist.name,))
msg = '%s requires more parameters to ' \
'compute ppf'.format(self.dist.name,)
print(msg)

This comment has been minimized.

Copy link
@josef-pkt

josef-pkt Dec 1, 2013

Member

we should use warnings.warn instead of print statements

should this raise an exception?

return self.dist.cdf(qntls)

def ppplot(self, xlabel=None, ylabel=None, line=None, other=None, ax=None):
def ppplot(self, ax=None, xlabel=None, ylabel=None, line=None, other=None,
**plotkwargs):

This comment has been minimized.

Copy link
@josef-pkt

josef-pkt Dec 1, 2013

Member

I'm not sure about moving the position of ax.
I don't worry too much about backwards compatibility in this case but we should have a consistent pattern across plots.

If given, this subplot is used to plot in instead of a new figure
being created.
**plotkwargs : additional matplotlib arguments to be passed to the

This comment has been minimized.

Copy link
@josef-pkt

josef-pkt Dec 1, 2013

Member

I don't think there should be a blank line before **plotkwargs

@@ -249,19 +268,24 @@ def ppplot(self, xlabel=None, ylabel=None, line=None, other=None, ax=None):

return fig

def qqplot(self, xlabel=None, ylabel=None, line=None, other=None, ax=None):
def qqplot(self, ax=None, xlabel=None, ylabel=None, line=None, other=None,
**plotkwargs):

This comment has been minimized.

Copy link
@josef-pkt

josef-pkt Dec 1, 2013

Member

same here about moving ax

Returns

This comment has been minimized.

Copy link
@josef-pkt

josef-pkt Dec 1, 2013

Member

accidental delete of Returns ?

fig1 = sm.qqplot_2samples(x, y, line=line)

plt.close('all')
class _base_probplot:

This comment has been minimized.

Copy link
@josef-pkt

josef-pkt Dec 1, 2013

Member

pep-8 naming Capitalization in class names
subclass object

add Mixin to end of class name to make pylint a bit happier

self.base_setup()


class test_top_level:

This comment has been minimized.

Copy link
@josef-pkt

josef-pkt Dec 1, 2013

Member

class name should be capitalized and now undelines

@phobson

This comment has been minimized.

Copy link
Contributor Author

commented Dec 1, 2013

@josef-pkt I addressed all of the comments. Decided to raise errors instead of vanilla print statements

@coveralls

This comment has been minimized.

Copy link

commented Dec 1, 2013

Coverage Status

Coverage remained the same when pulling e2694aa on phobson:ProbPlot-enhancements-and-tests into 9d4b1f8 on statsmodels:master.

@coveralls

This comment has been minimized.

Copy link

commented Dec 1, 2013

Coverage Status

Coverage remained the same when pulling e2694aa on phobson:ProbPlot-enhancements-and-tests into 9d4b1f8 on statsmodels:master.

Options for the reference line to which the data is compared:

This comment has been minimized.

Copy link
@josef-pkt

josef-pkt Dec 18, 2013

Member

removing this line will break sphinx/rst rendering, an item list needs to be preceded by a blank line (even if it doesn't always look nice in source)

This comment has been minimized.

Copy link
@josef-pkt

josef-pkt Dec 24, 2013

Member

blank line missing, needs doc build to check

"""
P-P plot of the percentiles (probabilities) of x versus the
probabilities (percetiles) of a distribution.
Parameters
----------
xlabel, ylabel : str or None
ax : Matplotlib AxesSubplot instance, optional

This comment has been minimized.

Copy link
@josef-pkt

josef-pkt Dec 18, 2013

Member

ax is now in the wrong position in docstring, given that it's again the last explicit keyword


def qqplot(self, xlabel=None, ylabel=None, line=None, other=None, ax=None):
def qqplot(self, xlabel=None, ylabel=None, line=None, other=None,
ax=None, **plotkwargs):

This comment has been minimized.

Copy link
@josef-pkt

josef-pkt Dec 18, 2013

Member

STY (minor) one space too many in intend

"""
Q-Q plot of the quantiles of x versus the quantiles/ppf of a
distribution or the quantiles of another `ProbPlot` instance.
Parameters
----------
xlabel, ylabel : str or None
ax : Matplotlib AxesSubplot instance, optional

This comment has been minimized.

Copy link
@josef-pkt

josef-pkt Dec 18, 2013

Member

also ax is now in wrong position in Parameters list

@@ -316,7 +332,8 @@ def qqplot(self, xlabel=None, ylabel=None, line=None, other=None, ax=None):

return fig

def probplot(self, line=None, ax=None, exceed=False):
def probplot(self, ax=None, xlabel=None, ylabel=None, line=None,

This comment has been minimized.

Copy link
@josef-pkt

josef-pkt Dec 18, 2013

Member

ax in second position,
we should have consistent sequencing of arguments

If given, this subplot is used to plot in instead of a new figure
being created.
excced : boolean
excced : boolean, optional

This comment has been minimized.

Copy link
@josef-pkt

josef-pkt Dec 18, 2013

Member

typo exceed



class TestProbPlotRandomNormalMinimal(BaseProbplotMixin):
def setup(self):

This comment has been minimized.

Copy link
@josef-pkt

josef-pkt Dec 18, 2013

Member

add random.seed here and at all the similar classes below

@@ -1,4 +1,5 @@
import numpy as np
np.random.seed(5)

This comment has been minimized.

Copy link
@josef-pkt

josef-pkt Dec 18, 2013

Member

random seed should not be on top of a module
should be in front of the random number generation to make it replicable. Rearranging tests will change random seeds used in the different tests, also nosetests runs tests in a sequence that will change if a new test is added.

@josef-pkt

This comment has been minimized.

Copy link
Member

commented Dec 18, 2013

I'm going to try out some examples, and would like to merge this today or this week.

Can you rebase on current master and force push? Otherwise, I will rebase before merging.

@phobson

This comment has been minimized.

Copy link
Contributor Author

commented Dec 18, 2013

@josef-pkt I'll try to get to this later tonight (west coast time). I appreciate all of your comments and rebasing won't be a problem for me.

@josef-pkt

This comment has been minimized.

Copy link
Member

commented Dec 18, 2013

Thanks, I'd like to merge this by Friday (before kids go on vacation and my attention span will become very short)

phobson added some commits Nov 10, 2013

ENH: moved probplot over to properties and added some kwargs to plott…
…ing fxns

Also reworked the tests.

BUG: fixed a 'call' to a property

BUG: fixed missing paren in sample_percentile equation

WIP: trying out this cache_readonly decorator

TST: beefier, class-based tests

TST: decorating methods, not test classes + fixed a function call

TST/BUG: fixed references to class attributes

TST/BUG: fixed references to class attributes (part2)

TST/BUG: using the right type of cache

TST/BUG: fixed references to class attributes (part3)
@coveralls

This comment has been minimized.

Copy link

commented Dec 22, 2013

Coverage Status

Changes Unknown when pulling f047d54 on phobson:ProbPlot-enhancements-and-tests into * on statsmodels:master*.

@phobson

This comment has been minimized.

Copy link
Contributor Author

commented Dec 22, 2013

@josef-pkt sorry that took so long.

@josef-pkt

This comment has been minimized.

Copy link
Member

commented Dec 22, 2013

Thanks, I try to merge it soon.

One git tip: you can set global config options to warn or remove trailing whitespace (in case they were yours)
One Windows automatic removing trailing whitespace on commit doesn't work. But it works during rebase, and when I rebase a branch then trailing whitespaces are corrected and removed from the commits.

@phobson

This comment has been minimized.

Copy link
Contributor Author

commented Dec 22, 2013

Are they there now? I've got sublime text configured to hightlingand trim
trailing white space

On Sun, Dec 22, 2013 at 5:45 AM, Josef Perktold notifications@github.comwrote:

Thanks, I try to merge it soon.

One git tip: you can set global config options to warn or remove trailing
whitespace (in case they were yours)
One Windows automatic removing trailing whitespace on commit doesn't work.
But it works during rebase, and when I rebase a branch then trailing
whitespaces are corrected and removed from the commits.


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

@josef-pkt

This comment has been minimized.

Copy link
Member

commented Dec 22, 2013

Are they there now?

I haven't checked and assume not. This was based on the commit messages about trailing whitespace.

@phobson

This comment has been minimized.

Copy link
Contributor Author

commented Dec 22, 2013

Ok. Gotcha. Thanks. That commit was when I was ironing on the configuration on a new virtual machine. --
Paul Hobson
Sorry if this is unintelligible. I'm on my phone.

On Sun, Dec 22, 2013 at 8:59 AM, Josef Perktold notifications@github.com
wrote:

Are they there now?

I haven't checked and assume not. This was based on the commit messages about trailing whitespace.

Reply to this email directly or view it on GitHub:
#1179 (comment)

@josef-pkt

This comment has been minimized.

Copy link
Member

commented Dec 24, 2013

Quickly reading through this, I don't see any problems anymore.

doc build will show if current plots in the documentation change, since the unit tests are only smoketests.
I don't see any changes that would be backwards compatible for using plots (cached attribute access is mostly internal)

josef-pkt added a commit that referenced this pull request Dec 24, 2013

Merge pull request #1179 from phobson/ProbPlot-enhancements-and-tests
REF/TST: `ProbPlot` now uses `resettable_cache` and added some kwargs to plotting fxns

@josef-pkt josef-pkt merged commit be7d4e9 into statsmodels:master Dec 24, 2013

1 check passed

default The Travis CI build passed
Details
@josef-pkt

This comment has been minimized.

Copy link
Member

commented Dec 24, 2013

merged

Thank you @phobson

@phobson phobson deleted the phobson:ProbPlot-enhancements-and-tests branch Dec 24, 2013

PierreBdR pushed a commit to PierreBdR/statsmodels that referenced this pull request Sep 2, 2014

Merge pull request statsmodels#1179 from phobson/ProbPlot-enhancement…
…s-and-tests

REF/TST: `ProbPlot` now uses `resettable_cache` and added some kwargs to plotting fxns
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.