search

GeoStat-Framework / PyKrige

Kriging Toolkit for Python
https://pykrige.readthedocs.io
BSD 3-Clause "New" or "Revised" License
759 stars 188 forks source link

Add ClassificationKriging #165

Closed mralbu closed 3 years ago

mralbu commented 4 years ago

Adds ClassificationKriging class adapting RegressionKriging to work with categorical variables. It maps the scikit-learn classifiers predict_proba simplexes to an isometric space using the ilr transform, and kriges/interpolates the residuals of the (also ilr transformed) observed data. I am far from being an expert on geostatistics or compositional data analysis, but I believe this method corresponds to the Simplicial Indicator Kriging method.

MuellerSeb commented 3 years ago

Hey @mralbu! Sorry for the late reply. Quite busy with my PhD these days. Thanks for this great extension! Classification Kriging is really a big plus for this package!

I got some remarks:

If you don't want to care about the refactor, just leave ClassificationKriging where it is and I will restructure the package afterwards. Just apply black (link) and I would love to see a plot for the example.

Hope you are ok with these remarks.

I will do a review in addition.

Cheers, Sebastian

mralbu commented 3 years ago

Hey @mralbu! Sorry for the late reply. Quite busy with my PhD these days. Thanks for this great extension! Classification Kriging is really a big plus for this package!

I got some remarks:

  • please apply black to your changes, so CI succeeds
  • I'd like to have a submodule called ck.py to import classification kriging from there. Then we could also refactor the sklearn interface to provide the Krige class used by rk and ck. Could you just put Krige in compat.py and import it from there?
  • It would be nice, if the example you provided could produce a plot, so it pops up in the generated gallery. Is it possible for you to make a simple plot to get an impression of the output?
  • I can care about further details afterwards, so we can come up with v1.5.2 wink

If you don't want to care about the refactor, just leave ClassificationKriging where it is and I will restructure the package afterwards. Just apply black (link) and I would love to see a plot for the example.

Hope you are ok with these remarks.

I will do a review in addition.

Cheers, Sebastian

Hi, Sebastian! Completely understand the delay.. I've recently finished my masters and, though much simpler, it did require my full attention for quite some time. I'll try to work on the pull request this weekend. I appreciate your revision and suggestions. I understand it is a large pull request and feature addition, at least for what I am used to.

mralbu commented 3 years ago

Haven't figured out a good plot yet for example 10_classification_kriging2d.py. The README.rst description may need to be improved as well. Instead of Simplifical Indicator kriging can be performed with pykrige.rk.ClassificationKriging., maybe a better description, though a little more intricate, could be Simplifical Indicator kriging of ilr transformed classification residuals can be performed with pykrige.rk.ClassificationKriging. Not sure what would be best here.. What do you think?

MuellerSeb commented 3 years ago

@mralbu Lost track of this again. Sorry. Let's get this merged. I will just do some cleanup.

I am fine with no plot (other examples also don't have one). I like the more explicit description for the README more. Will add it with a link for the ilr transform.

pjuangph commented 3 years ago

Is this feature still going to be merged? Just wondering.

MuellerSeb commented 3 years ago

Will add these changes to a new branch, so I can take over. Thanks again @mralbu for your work!