-
Notifications
You must be signed in to change notification settings - Fork 188
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
Grid search update #158
Grid search update #158
Conversation
…tible with all kriging routines and parameters
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
pykrige/rk.py
Outdated
self.model = None # not trained | ||
self.n_closest_points = n_closest_points | ||
self.method = method | ||
self.val_kw = "val" if self.method in threed_krige else "z" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do so.
external_drift_x=self.external_drift_x, | ||
external_drift_y=self.external_drift_y, | ||
functional_drift=self.functional_drift, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following the Zen of Python: Explicit is better than Implicit ;-)
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. |
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. |
Then I will do this for anisotropy scaling and angles. Thanks for your thoughts on that! |
Any chance we could also aggregate,
into one parameter? |
That is true, I can do that. We could call it
|
@rth ping. :-) |
I think, I'll just merge this by next week! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @MuellerSeb , LGTM.
This PR updates the sklearn wrapper and the Regression Kriging class in order to be compatible with all kriging routines.
resolves #128