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

DOC: document default argument values for some arpack functions #3851

Merged
merged 1 commit into from
Aug 13, 2014

Conversation

argriffing
Copy link
Contributor

closes #3829

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 5b7d002 on argriffing:arpack-default-docstrings into 8741fe1 on scipy:master.

@argriffing
Copy link
Contributor Author

TravisCI had two errored configurations. One failed to download dependencies, and another stalled for 10min during a test.

@@ -1111,9 +1111,11 @@ def eigs(A, k=6, M=None, sigma=None, which='LM', v0=None,

v0 : ndarray, optional
Starting vector for iteration.
ncv : int, optional
Default: random
ncv : integer, optional
Copy link
Member

Choose a reason for hiding this comment

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

this change, and similar ones below, aren't right. The type descriptor should use the actual type here (except if it's array_like), so int and not integer. See section 4 under https://github.com/numpy/numpy/blob/master/doc/HOWTO_DOCUMENT.rst.txt#sections

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've seen both ways, so I was just guessing. Does integer have any meaning? For example hypothetically it might be more flexible allowing int and np.int16 and whatever else acts like an integer, whereas int might hypothetically mean exactly Python int type. If not then I'll change them all to int.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed this back to int, and made some more integer->int changes.

Copy link
Member

Choose a reason for hiding this comment

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

No, integer doesn't have an extended meaning. In a sentence in the parameter description you would use integer (because it's a normal word) and in the type descriptor you always use int.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) when pulling 9ce88c6 on argriffing:arpack-default-docstrings into 8741fe1 on scipy:master.

@@ -1111,9 +1111,11 @@ def eigs(A, k=6, M=None, sigma=None, which='LM', v0=None,

v0 : ndarray, optional
Starting vector for iteration.
Default: random
Copy link
Member

Choose a reason for hiding this comment

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

Looks to me like default is all zeros, not random. See v0 handling in _ArpackParams.

Copy link
Member

Choose a reason for hiding this comment

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

Same comment for v0 in other two functions below.

@rgommers
Copy link
Member

ncv and maxiter look correct to me.

Could you squash the commits after updating?

@rgommers rgommers added this to the 0.15.0 milestone Aug 13, 2014
@argriffing
Copy link
Contributor Author

Looks to me like default is all zeros, not random. See v0 handling in _ArpackParams.

Are you sure? I remember having thought the same thing as well, and having spent a lot of time considering this possibility, and concluding that it's actually random and not zeros. I remember having tested this by feeding a v0 that was like np.zeros and this resulted in an exception like "error v0 is all zeros". I think there's a deeper layer that uses a random initial vector if v0 is unspecified at the scipy level.

@rgommers
Copy link
Member

I didn't test, just followed the code. It seems simple enough. The only placev0 is used is in lines 317-322:

    if v0 is not None:
        # ARPACK overwrites its initial resid,  make a copy
        self.resid = np.array(v0, copy=True)
        info = 1
    else:
        self.resid = np.zeros(n, tp)
        info = 0

Feeding in v0 = np.zeros(n, tp) should not give you an exception I'd think.

EDIT: or it depends on the info flag.

@rgommers
Copy link
Member

Indeed, you are right. A comment behind info = 0 in arpack.py wouldn't hurt.

@rgommers
Copy link
Member

Comment in dsaupd.f:

c  INFO    Integer.  (INPUT/OUTPUT)
c          If INFO .EQ. 0, a randomly initial residual vector is used.
c          If INFO .NE. 0, RESID contains the initial residual vector,
c                          possibly from a previous run.

@argriffing
Copy link
Contributor Author

EDIT: or it depends on the info flag.

Yes I spent some time tracking through the arpack maze, and I think it uses the info flag, eventually using a random initial vector. If you try to give it zeros directly, it tells you to use a more useful initial vector.

@argriffing
Copy link
Contributor Author

Could you squash the commits after updating?

I think I've done this. It was scary using the force flag in git push -f so I hope it's not screwed up.

@argriffing
Copy link
Contributor Author

A comment behind info = 0 in arpack.py wouldn't hurt.

Added in the squashed commit.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling e539378 on argriffing:arpack-default-docstrings into 47137df on scipy:master.

@pv pv removed the PR label Aug 13, 2014
@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling e539378 on argriffing:arpack-default-docstrings into 47137df on scipy:master.

@rgommers
Copy link
Member

Thanks, merging.

rgommers added a commit that referenced this pull request Aug 13, 2014
DOC: document default argument values for some arpack functions
@rgommers rgommers merged commit 704b554 into scipy:master Aug 13, 2014
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.

Linear algebra function documentation doesn't mention default parameter values
5 participants