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

GSTools covariance model as variogram_model input #125

Merged
merged 14 commits into from
Jan 28, 2020
Merged

GSTools covariance model as variogram_model input #125

merged 14 commits into from
Jan 28, 2020

Conversation

MuellerSeb
Copy link
Member

With this pull request an interface to GSTools is provided. The PyKrige routines UniversalKriging, UniversalKriging3D, OrdinaryKriging and OrdinaryKriging3D now allow an instance of the GSTools CovModel as input for variogram_model. Every information needed will be set internally.

An example was added to the README and to the example folder.

@MuellerSeb
Copy link
Member Author

I went through the CI logs and I think the check-fails are not induced by the pull-request related changes.
Is there something broken in your CI setup or did I miss something?
Cheers,
Sebastian

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, this is very nice!

A few comments about the code,

  • no need to include examples/gstools_example.png, it will be generated while building the docs in https://pykrige.readthedocs.io/en/latest/examples/
  • CI is failing because gstools need to be installed in CI before examples are run.
  • It would be good to add optional tests to CI, something along the lines of,
    pykrige/tests/test_core.py
    @pytest.mark.paramterize(model, [OrdinaryKriging, UniversalKriging])
    def test_gstools_variogram(model):
        gstools = pytest.importorskip("gstools")
      
        # create some data
        # create the gstools model
        # fit the krigging estimator

pykrige/ok.py Outdated Show resolved Hide resolved
pykrige/ok3d.py Outdated Show resolved Hide resolved
pykrige/ok3d.py Outdated Show resolved Hide resolved
pykrige/uk.py Outdated Show resolved Hide resolved
examples/gstools_covmodel.py Outdated Show resolved Hide resolved
@MuellerSeb
Copy link
Member Author

Thanks to @LSchueler I could resolve the diff issue. Now I can care about your suggestions. Sorry for the long delay, but I was a bit feared of the rebase.

@MuellerSeb
Copy link
Member Author

Ping.

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! A few more comments. The first two (about factorizing code and linking in docstring) apply to all files where such changes are made.

which to rotate coordinate system in order to take into
account anisotropy. Default is 0 (no rotation).
"""

# set up variogram model and parameters...
self.variogram_model = variogram_model
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we factorize this code repeated in all estimators, maybe as,

def _setup_variogram_parameters(self, ..)

under pykrige.core. There is already an _initialize_variogram_model there. All that could probably be made methods of some base class, but that should probably be done in a separate PR..

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 just tried that, but it turns out to be more difficult than thought. It should be done in a separate PR refactoring the kriging classes. The update_variogram_model method should be used here, but it has to be adopted, to work with different dimensions. I will leave it as it is now, if that is ok.

pykrige/ok3d.py Outdated
Specified which variogram model to use; may be one of the following:
linear, power, gaussian, spherical, exponential, hole-effect.
Default is linear variogram model. To utilize a custom variogram model,
specify 'custom'; you must also provide variogram_parameters and
variogram_function. Note that the hole-effect model is only technically
correct for one-dimensional problems.
You can also use a GSTools
CovModel, see https://github.com/GeoStat-Framework/GSTools
Copy link
Contributor

Choose a reason for hiding this comment

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

Use RST link instead of raw url

`GSTools <https://github.com/GeoStat-Framework/GSTools>`_

README.rst Outdated
@@ -31,6 +31,10 @@ point and all grid points. With the 'functional' drift capability, the user may
of the spatial coordinates that define the drift(s). The package includes a module that contains functions
that should be useful in working with ASCII grid files (`*.asc`).

PyKrige also works smoothly together with the Covariance Model provided by
`GSTools <https://github.com/GeoStat-Framework/GSTools>`_ v1.1 and higher.
Have a look at the section: `GSTools covariance models`_.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would remove this last sentence, as a link to the documentation of another package before the link to pykrige documentation is confusing. User who need this functionality would be able to find the instructions from other links in this PR.

@rth
Copy link
Contributor

rth commented Nov 13, 2019

Also it looks like CI on master may also need fixing (the CI failures are likely unrelated to this PR). Would you be interested in opening a separate PR to look into it?

Sorry, this package is very little maintained: personally I'm no longer working on krigging related tasks, and we won't be able to merge this PR as long as CI is not green (at least for Python 3.5+).

@MuellerSeb
Copy link
Member Author

@rth I can have a look at the CI and make a separate PR.

@MuellerSeb
Copy link
Member Author

Also it looks like CI on master may also need fixing (the CI failures are likely unrelated to this PR). Would you be interested in opening a separate PR to look into it?

Done.

Sorry, this package is very little maintained: personally I'm no longer working on krigging related tasks, and we won't be able to merge this PR as long as CI is not green (at least for Python 3.5+).

Again, I could offer help with maintaining this package by integrating it to the GeoStat-Framework. Since it's an github organization for geostatics in python, it is easy to share maintainance of packages related to this topic.. at least that is what we are aiming at.
We can discuss about that if you want. Don't want to obtrude myself upon you.

@MuellerSeb
Copy link
Member Author

should be ready now.

@rth
Copy link
Contributor

rth commented Dec 2, 2019

I'll try to have a look later this week at the PR.

Again, I could offer help with maintaining this package by integrating it to the GeoStat-Framework. Since it's an github organization for geostatics in python, it is easy to share maintenance of packages related to this topic.. at least that is what we are aiming at.

Maybe try contacting @bsmurphy by email about it and cc me? At least to see if he could give you write access to this repo. Another solution could be to mark this repo as unmaintained with a link to a maintained version at that org. In any case, personally I won't have much availability to help with the maintenance of this package in the future, and it would be good if some solution could be found at least to merge the occasional PR from contributors (also a PhD is a good time to contribute to OSS :) )

@MuellerSeb
Copy link
Member Author

ping.

Copy link
Member Author

@MuellerSeb MuellerSeb left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@MuellerSeb MuellerSeb merged commit 40a8140 into GeoStat-Framework:master Jan 28, 2020
@MuellerSeb MuellerSeb mentioned this pull request Apr 2, 2020
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.

2 participants