Skip to content

Conversation

@mralbu
Copy link
Contributor

@mralbu mralbu commented Aug 31, 2020

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.

@mralbu mralbu mentioned this pull request Sep 2, 2020
@MuellerSeb
Copy link
Member

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 😉

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

Copy link
Member

@MuellerSeb MuellerSeb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got some ideas for vectorization. Overall looks good to me. I just need some explanations for the eps hack. 😉

@@ -0,0 +1,49 @@
"""
Classification kriging
------------------
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Too few "-" to create a headline.

from pykrige.ok import OrdinaryKriging
from pykrige.compat import validate_sklearn

validate_sklearn()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is tricky. validate_sklearn was called before all sklearn import in order to raise a specific Error if it was not installed. But as I said: I can care about that later.

@MuellerSeb MuellerSeb self-assigned this Nov 18, 2020
@MuellerSeb MuellerSeb added this to the v1.5.2 milestone Nov 18, 2020
@mralbu
Copy link
Contributor Author

mralbu commented Nov 19, 2020

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 mralbu force-pushed the classification-kriging branch 2 times, most recently from 3be4999 to c47df3d Compare November 20, 2020 03:10
@mralbu
Copy link
Contributor Author

mralbu commented Nov 20, 2020

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?

@mralbu mralbu force-pushed the classification-kriging branch from c47df3d to d30c718 Compare November 20, 2020 03:23
@MuellerSeb
Copy link
Member

@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
Copy link

pjuangph commented Apr 1, 2021

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

@MuellerSeb MuellerSeb changed the base branch from develop to classification-kriging April 1, 2021 15:56
@MuellerSeb
Copy link
Member

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

@MuellerSeb MuellerSeb merged commit f909f41 into GeoStat-Framework:classification-kriging Apr 1, 2021
This was referenced Apr 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants