Added a couple of methods to obtain the variogram model points or to save them to a file #94
Conversation
pykrige/ok.py
Outdated
| """ | ||
| variogram_points = self.variogram_function(self.variogram_model_parameters, self.lags) | ||
|
|
||
| return np.array([self.lags, variogram_points]) |
There was a problem hiding this comment.
I don't really see the advantage of concatenating lags and variogram points in term of the API.
Maybe just return the variogram_points would be sufficient? lags can already be accessed as an attribute.
There was a problem hiding this comment.
Well, I'll look into that, where can I read more about the API? I'd like to be in full compliance with it.
There was a problem hiding this comment.
lags is a class attribute that I don't think is documented very well internally (I don't think it's mentioned in any of the docstrings...). You need the lags to do anything with the variogram points, so I actually don't mind returning them both.
There was a problem hiding this comment.
I thought it would be better to return them both too, since it makes easier the use of the data points
pykrige/ok.py
Outdated
| @@ -404,6 +404,34 @@ def display_variogram_model(self): | |||
| self.lags), 'k-') | |||
| plt.show() | |||
|
|
|||
| def get_variogram_points(self): | |||
| """Obtain the data pairs for the variogram model. | |||
| returns a numpy.array with the form [lag, variogram_value_at_lag] | |||
There was a problem hiding this comment.
The type of return variable(s) should be documented see e.g. https://github.com/bsmurphy/PyKrige/blob/4c54e28da59dccd3620e586cf38e88f56a4c5eca/pykrige/ok.py#L556
There was a problem hiding this comment.
I will add docstrings for that
pykrige/ok.py
Outdated
|
|
||
| return np.array([self.lags, variogram_points]) | ||
|
|
||
| def save_variogram_to_file(self, output_file, separator=" "): |
There was a problem hiding this comment.
I'm not so keen on this addition.
Assuming the above changes are made, this could be done fairly easily with,
import pandas as pd
df = pd.DataFrame({'lag': model.lag,
'variogram_points': model.get_variogram_points()})
df.to_csv('<some-file>)there is not need to include and maintain a custom CSV like format in pykrige for this..
There was a problem hiding this comment.
You are right, i'll take it out
|
This looks good, thanks for contributing! Agreed the biggest addition we'd ask for is an extended docstring, so it'll play nice with our readthedocs page. Of course, a lot of the I think AppVeyor might be crashing for an unrelated reason, so once you push an update we'll see what happens. |
|
I'd like to help on the docstrings side, maybe I don't know the API well, but I could help with styling or infering what each of the method does |
|
As for AppVeyor, I figured the problem is with the conda environment for Python 3.5, I did looked what happened, but can't remember the specifics now |
|
With the docstrings, we're trying to follow the numpy docstring convention: https://numpydoc.readthedocs.io/en/latest/format.html#docstring-standard Most of the main routines ( |
We can ignore the failed Appveyor test, it's a known issue that is fixed on master in 0569194
We don't really have a write-up for API principles, so it mostly a discussion at review time. For my part I think it would make sense to adapt as much as possible of scikit-learn's API design. Also if you are integeresed in general ideas about the scipy API principles, see the second part of this presentation particularly starting from slide 60. |
|
Hey, sorry for the late commits, I've added a couple of tests for the method BTW, the slides you showed me are amazing, is it there a video for them? could'nt find it myself Daniel. |
Daniel-M
changed the title
|
Looks good to me! I think the name is good, fits with the general convention we have going. @rth might have some comments, but I'd say we can go ahead and merge once he takes a look. |
|
@rth, can we merge this? |
|
Sorry for the late response, yes this LGTM, merging. Thanks @Daniel-M ! |
Hi, I've added a couple of methods to UniversalKriging and OrdinaryKriging:
get_variogram_points: wich returns a numpy.array[lags, variogram_value_at_lags]which can be used of interest for the user (for further computations).save_variogram_to_file: wich saves the variogram to a fileoutput_filewith separatorseparator(which defaults to spacelag variogram_value_at_lagaccording to the separator. Making a call with separator="," would save the values aslag,variogram_value_at_lag.