Grid search update

[MuellerSeb]

This PR updates the sklearn wrapper and the Regression Kriging class in order to be compatible with all kriging routines.
resolves #128

[rth]

Thanks @MuellerSeb , generally looks good but I'm a bit concerned by the number of added parameters. Can't we make for instance,

        anisotropy_angle_x=0.0,
        anisotropy_angle_y=0.0,
        anisotropy_angle_z=0.0,

just,

anisotropy_angle = (0, 0, 0)

that would also be easier to generalize to 1D/2D/3D?

[rth]

This would technically fail scikit-learn's validation check in check_no_attributes_set_in_init. It might be better to set it in fit, and also maybe make it private _val_kw if users are not expected to interact with it?

[MuellerSeb]

Will do so.

[rth]

At this point maybe,

add_setup = {key: getattr(self, key) for key in ["anisotropy_scaling", ...]}

but writing is explicitly as done above is also fine..

[MuellerSeb]

Following the Zen of Python: Explicit is better than Implicit ;-)

[MuellerSeb]

Thanks @MuellerSeb , generally looks good but I'm a bit concerned by the number of added parameters. Can't we make for instance,

        anisotropy_angle_x=0.0,
        anisotropy_angle_y=0.0,
        anisotropy_angle_z=0.0,

just,

anisotropy_angle = (0, 0, 0)

that would also be easier to generalize to 1D/2D/3D?

This is indeed true and it is the way it is done in GSTools for example. For PyKrige v2 we should definitly do it this way and since I want to use the GSTools CovModel then, it will be simplified either ways.
For the moment I would keep it this way, since the signatures of the kriging classes are this way. In most cases these parameters can be ignored and the class can be used in the way it was before. So for the 1.5.1 I would keep it.

[rth]

For the moment I would keep it this way, since the signatures of the kriging classes are this way.

Right, but introducing a new parameter knowing that it will be deprecated in next version and users will have to change is not ideal. If Krigging class was another subtype of krigging I would agree about API consistency but here it's aims to be a general top level abstraction for krigging. I think it would be better to directly do small improvements in the direction of v2 for this class, rather than break things in a major way between v1 and v2 if we can help it.

[MuellerSeb]

Then I will do this for anisotropy scaling and angles. Thanks for your thoughts on that!

[rth]

Any chance we could also aggregate,

        self.external_drift = external_drift
        self.external_drift_x = external_drift_x
        self.external_drift_y = external_drift_y

into one parameter?

[MuellerSeb]

Any chance we could also aggregate,

        self.external_drift = external_drift
        self.external_drift_x = external_drift_x
        self.external_drift_y = external_drift_y

into one parameter?

That is true, I can do that. We could call it ext_drift_grid.
But there are also still some parameters left:

• exact_values (all routines)
• specified_drift (uk and uk3d) -> but this one is a bit delicate, since it would need the specified_drift_arrays in the execute method, which is hard to manage in when random samples are used for training. I'm not familiar enough with sklearn to know if this could be done. @rth : should we skip it then?

[MuellerSeb]

@rth ping. :-)

[MuellerSeb]

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

[rth]

Thanks @MuellerSeb , LGTM.