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

[Refactor] prefere "cor" to specify userdefined CovModel #90

Closed
MuellerSeb opened this issue May 9, 2020 · 8 comments · Fixed by #109
Closed

[Refactor] prefere "cor" to specify userdefined CovModel #90

MuellerSeb opened this issue May 9, 2020 · 8 comments · Fixed by #109
Assignees
Labels
Refactoring Code-Refactoring needed here
Milestone

Comments

@MuellerSeb
Copy link
Member

At the moment we allow to specify one of the following functions for a user-defined model:

  • variogram - full variogram including variance, nugget, length-scale and potential additional parameters
  • covariance - covariance only including variance, length-scale and potential additional parameters
  • correlation - correlation only including length-scale and potential additional parameters
  • cor - normalized correlation depending on the non-dimensional distance only including potential additional parameters

Only the cor method is a save way of defining a fully working CovModel which results in the standard definition from the documentation, so we should think about sticking to this one as the standard way of defining a CovModel.

@MuellerSeb MuellerSeb added the Refactoring Code-Refactoring needed here label May 9, 2020
@MuellerSeb MuellerSeb added this to the 1.3 milestone May 9, 2020
@mmaelicke
Copy link
Member

Could you elaborate on why only cor is a save way? I'm not into CovModel that much.

In Terms of GS-Framework v2 this might be helpful to limit the user-defined models of CovModel to cor. A user-defined variogram can then be used to define a potential gstools.Variogram instance. Would make it way easier to map between a skgstat.Variogram and gstools.Variogram, or merging the two at that point, though. (should go into another issue)

There are also instances, where a covariance function is not defined, but a variogram is. From my point of view, it would also be a more clearer notation if not any variogram can be used to define a CovModel. In these cases any kind of Variogram class can be used to decide if a CovModel would be defined or not.

@MuellerSeb
Copy link
Member Author

The only valid variograms models, that we are excluding, are the unbounded ones. So if we want to support unbounded variograms, I think this should be represented by a new class.

The RandMeth class only takes bounded variograms to generate randomfields, since it is depending on the spectral density of the covariance. And the covariance only exists for bounded variograms. In addition, the spectral density is only a real "density" function, if the covariance/varigram model is valid (conditional negative semidefinite).

If we implement unbounded variograms, we should also provide a SRF generator for these. (Sequential Gaussian, Turning bands, ... something like this)

@MuellerSeb
Copy link
Member Author

MuellerSeb commented May 9, 2020

@mmaelicke : The variogram in GSTools is represented by:

vario(r) = var * (1 - cor(r / len_scale)) + nugget

If you use the CovModel class to create your own variogram, and you override the variogram method to define it, you have to take care of variance, len_scale and nugget, so everything is still working (fitting, random field generation, integral_scale resetting and so on...)

If you only define cor, you don't have to care about all the other parameters. From the example gallery:

class Gau(gs.CovModel):
    def cor(self, h):
        return np.exp(-h ** 2)

This reproduces the gaussian variogram.

@MuellerSeb MuellerSeb added this to To do in GS-Framework v2 May 9, 2020
@mmaelicke
Copy link
Member

@MuellerSeb ah, ok. Got it. Will be interesting how we bring scikit-gstat to play well here in a v2.... But that's not the issue here.

From what you write above, I would let the user define only cor. Making sure that variance and len_scale are always valid could be an obstacle for some users, implementing cor should be more straightforward.

On the other hand, if you feel more comfortable using variograms (like taught in some gestatisitics classes) it would still be nice to use CovModel. So before variogram gets deprecated, a helper function or working interface between a new Variogram class, or scikit-gstat, or both would be nice, from my personal point of view. Ultimately not because one would not be able to define a covariance or correlation function, but maybe not used to it.

@MuellerSeb
Copy link
Member Author

MuellerSeb commented May 30, 2020

This approach also allows to implement a rescaling factor R, since a lot of different definitions exist for the same variogram.
Then the varigram would be defined as:

vario(r) = var * (1 - cor(R * r / len_scale)) + nugget

This could be used to normalize the range, to perform a unit conversion or to use the earth radius with the yadrenko models (see #54).

See also: GeoStat-Framework/PyKrige#119

@MuellerSeb
Copy link
Member Author

... So before variogram gets deprecated...

@mmaelicke: Just a note: the variogram method of the CovModel won't be deprecated. We just disable the way of a user defined model by a given variogram function. By demanding the cor function, we assure, that the model is second-order stationary and all parameters are used correctly.
variogram, covariance and correlation will still be available and calculated internally from the cor method.
;-)

@mmaelicke
Copy link
Member

@MuellerSeb, thanks for clarifying.
To me, in terms of defining covariance models, this seems absolutely reasonable. In terms of scikit-gstat, it does not really make a difference as the CovModel can be unfitted (or not yet fitted) at definition and instantiation time, whereas skgstat.Variogram may not. Therefore, using the CovModel.variogram function for an interface would not have been a good idea anyway.

@MuellerSeb MuellerSeb changed the title [Refactor] only allow "cor" to specify userdefined CovModel [Refactor] prefere "cor" to specify userdefined CovModel Nov 11, 2020
@MuellerSeb
Copy link
Member Author

@mmaelicke I will keep all methods. Just be sure what you are doing! See #109

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Refactoring Code-Refactoring needed here
Projects
Development

Successfully merging a pull request may close this issue.

3 participants