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

Use pseudo inverse to prevent singular matrices #151

Merged
merged 14 commits into from
Aug 18, 2020
Merged

Conversation

MuellerSeb
Copy link
Member

@MuellerSeb MuellerSeb commented May 5, 2020

This is adressing #87, #150 and others.
The pseudo-inverse eventually solves the system by the least square error solution of the kriging equation. This helps to solve over-/under-determined systems.

If there are redundant points, their values will be averaged in the resulting krige field.

This feature can be pluged in via a switch in the init routines of Ordinary and Universal kriging in 2D/3D.

@MuellerSeb MuellerSeb self-assigned this May 5, 2020
@MuellerSeb MuellerSeb marked this pull request as draft May 5, 2020 14:55
@MuellerSeb MuellerSeb requested review from bsmurphy and rth May 5, 2020 14:56
@MuellerSeb MuellerSeb added this to the v1.5.1 milestone May 5, 2020
@MuellerSeb
Copy link
Member Author

And another note:
The example uses a linear model, that is not a valid one in 2D.

@rth
Copy link
Contributor

rth commented May 5, 2020

Are we confident that the matrix we are trying to invert is always Hermetian/symmetric?

Also at least in _exec_vector it looks like we could solve the linear system directly with scipy.linalg.solve which should be faster than inverting the matrix, or am I missing something?

@MuellerSeb
Copy link
Member Author

Yepp, the symmetry of the kriging matrix is a property we can use. pinvh is a lot faster than pinv for big matrices.

We could also solve it, but we should use scipy.linalg.lstsq, to guarantee a unique solution (should coincide with the pseudo-inverse solution)

I could check, if that solves the problem above.

@MuellerSeb
Copy link
Member Author

Problem is:
linalg.solve and linalg.lstsq don't support masked arrays. So this would need more refactoring.

@MuellerSeb
Copy link
Member Author

For some strange reason, one test is failing with pinvh but succeeds with pinv... so there are some numerical errors occurring. I would stick to pinv then.

@rth since the inversion in _exec_vector and _exec_loop is only done once, I think it would be ok to not use linalg.solve.

I would add a switch to use the pseudo-inverse, so the current behavior stays the same and user can enable the pseudo-inverse, if they need it.

@MuellerSeb
Copy link
Member Author

MuellerSeb commented Jun 22, 2020

I will implement a switch inv in the execute method to select the used method.
Either:

  • inv="inv" (default) use the inverse as before
  • inv="pinv" use pinv from scipy which uses lstsq to compute the pseudo-inverse
  • inv="pinv2" use pinv2 from scipy which uses singular value decomposition to compute the pseudo-inverse
  • inv="pinvh" use pinvh from scipy which uses the eigenvalues to compute the pseudo-inverse

Then everything will stay as it is and the users can select other routines for the inversion.

@MuellerSeb MuellerSeb marked this pull request as ready for review July 15, 2020 17:48
@MuellerSeb
Copy link
Member Author

I now decided to add two new keywords to the Kriging class __init__ routines, following GeoStat-Framework/GSTools#97:

    pseudo_inv : :class:`bool`, optional
        Whether the kriging system is solved with the pseudo inverted
        kriging matrix. If `True`, this leads to more numerical stability
        and redundant points are averaged. But it can take more time.
        Default: False
    pseudo_inv_type : :class:`int`, optional
        Here you can select the algorithm to compute the pseudo-inverse matrix:

            * `1`: use `pinv` from `scipy` which uses `lstsq`
            * `2`: use `pinv2` from `scipy` which uses `SVD`
            * `3`: use `pinvh` from `scipy` which uses eigen-values

So the current behavior is conserved and the usage of the pseudo-inverse can be opted in.

@rth : Are you OK with that?

@MuellerSeb MuellerSeb removed the request for review from bsmurphy July 16, 2020 13:13
@MuellerSeb
Copy link
Member Author

@rth ping. :-)

@MuellerSeb
Copy link
Member Author

I think, I'll just merge this by next week!

Copy link
Contributor

@rth rth left a comment

Choose a reason for hiding this comment

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

Thanks @MuellerSeb , sorry for late response. Generally LGTM but I haven't done a very thorough review.

One comment is that it might be better to use string options for pseudo_inv_type, as UniversalKrigging(pseudo_inv_type='pinvh') is more explicit/readable than UniversalKrigging(pseudo_inv_type=3).

pykrige/uk.py Outdated Show resolved Hide resolved
@MuellerSeb MuellerSeb merged commit 83ae0a8 into develop Aug 18, 2020
@MuellerSeb MuellerSeb deleted the pseudo_inv branch August 19, 2020 14:07
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.

2 participants