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

add relative_error and project_array functions #410

Merged
merged 7 commits into from
Nov 23, 2017

Conversation

sdrave
Copy link
Member

@sdrave sdrave commented Nov 16, 2017

No description provided.

@sdrave sdrave added this to the 0.5 milestone Nov 16, 2017
gramian = basis.gramian(product)
rhs = basis.inner(U, product)
coeffs = np.linalg.solve(gramian, rhs).T
return basis.lincomb(coeffs)
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason why not use basis = gram_schmidt(basis, product=product, atol=0, rtol=0) here? I would guess gramian could be ill-conditioned.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point! A problem with gram_schmidt might be that one would have to make a copy of basis, which potentially requires a lot of memory. Not sure what to prefer ...

Copy link
Member Author

Choose a reason for hiding this comment

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

The user could always call gram_schmidt outside and then set orthonormal to True. How about keeping the current code and additionally computing the condition of gramian? Then one could issue a warning, if the condition is too large. - On the other hand, the condition probably gets large very quickly. Any opinions, @pmli?

Copy link
Member

Choose a reason for hiding this comment

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

@sdrave How about adding copy=False to the gram_schmidt call? (That would actually make more sense than my first suggestion.)
(BTW, since recently, scipy.linalg.solve raises a warning if the system is ill-conditioned.)

Copy link
Member Author

Choose a reason for hiding this comment

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

copy=False is not really an option. The caller really would not expect basis to change, just by computing a projection.

Copy link
Member

Choose a reason for hiding this comment

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

Aha, ok. Then replacing numpy.linalg.solve by scipy.linalg.solve would be good.

SciPy's version will warn about ill-conditioned gramians and should be
slightly faster.
else:
gramian = basis.gramian(product)
rhs = basis.inner(U, product)
coeffs = scipy.linalg.solve(gramian, rhs, sym_pos=True, overwrite_a=True, overwrite_b=True).T
Copy link
Member

Choose a reason for hiding this comment

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

sym_pos=True should probably be replaced by assume_a='pos' (see here). But actually, is gramian guaranteed to be numerically symmetric and positive definite? Adding gramian = (gramian + gramian.T) / 2 might be a good idea, and solve will probably detect if gramian is close to being singular.

Copy link
Member Author

Choose a reason for hiding this comment

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

I used sym_pos instead of assume_a since the latter is only present in the latest SciPy release and I didn't want to make the code more complicated. - I guess in presence of a product, there is no guarantee that gramian is exactly spd. However, why do you think, that gramian = (gramian + gramian.T) / 2 would improve the result of the solve call in that case?

Copy link
Member

Choose a reason for hiding this comment

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

I guess it's mostly for my own sanity. But, it's nice that (A+A^T)/2 is the closest symmetric matrix to A. Anyway, Cholesky factorization can be computed in a stable way, so there shouldn't be a huge difference. Probably the bigger issue is that the condition number of gramian is the square of the condition number of basis (at least in the standard inner product).
Anyhow, calling gram_schmidt before avoids all of this, so feel free to ignore my previous comments.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I would also doubt that A+A^T/2 has an important effect. Merging as is ..

@sdrave sdrave merged commit 46db3cb into master Nov 23, 2017
@sdrave sdrave deleted the error_and_project_array branch November 23, 2017 14:13
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.

None yet

2 participants