search

sustainability-lab / polire

Spatial Interpolation in Python
https://sustainability-lab.github.io/polire/
BSD 3-Clause "New" or "Revised" License
12 stars 3 forks source link

suggestions by deepak #13 #18

Closed apoorvagnihotri closed 5 years ago

apoorvagnihotri commented 5 years ago

@sdeepaknarayanan When you get time do review the changes that I have made. Especially the change in idw.py.

sdeepaknarayanan commented 5 years ago

Can I know why we are adding XgBoost and Random Forest regressors? Are they related to Spatial Interpolation (We can have a base Neural Net Class too - a multi layer perceptron as well then). What's the motivation when we are dealing with classical interpolation methods?

apoorvagnihotri commented 5 years ago

We wanted to try out these methods on the data, therefore, I had added them. They seem to perform better than the other methods currently (see examples/air_dat.ipynb).

-- Apoorv Agnihotri Computer Science Department Final year Undergraduate IIT Gandhinagar apoorvagnihotri.github.io

Sent using Android.

On Thu, Jul 25, 2019, 11:57 AM S Deepak Narayanan notifications@github.com wrote:

Can I know why we are adding XgBoost and Random Forest regressors? Are they related to Spatial Interpolation (We can have a base Neural Net Class too - a multi layer perceptron as well then). What's the motivation when we are dealing with classical interpolation methods?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/sustainability-lab/polire/pull/18?email_source=notifications&email_token=AFMSG2P7N722RJWXNJZ5HELQBFBWNA5CNFSM4IGAQFQ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2YPVSI#issuecomment-514915017, or mute the thread https://github.com/notifications/unsubscribe-auth/AFMSG2N4MWRQSJTUJ5DO5ZDQBFBWNANCNFSM4IGAQFQQ .

sdeepaknarayanan commented 5 years ago

Can you make your data public? I can't see it in the branch for this pull request. Also, I think there was an issue with bspline (from ..utils import find_closest) Where is this find_closest? Also mention what correlation metric is used in air_dat.ipynb? I'd prefer using Spearman, or Pearson from Scikit Learn. Some correlation metrics aren't relevant for spatial interpolation (I think Kendall Tau is one of them). Can you clarify on this too?

apoorvagnihotri commented 5 years ago

The data is ~130 MB. I can send you a link.

Bspline issue can you be more specific?

-- Apoorv Agnihotri Computer Science Department Final year Undergraduate IIT Gandhinagar apoorvagnihotri.github.io

Sent using Android.

On Thu, Jul 25, 2019, 12:12 PM S Deepak Narayanan notifications@github.com wrote:

Can you make your data public? I can't see it in the branch for this pull request. Also, I think there was an issue with bspline (

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/sustainability-lab/polire/pull/18?email_source=notifications&email_token=AFMSG2LURM7JUSJ2OEOM5TTQBFDMFA5CNFSM4IGAQFQ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2YQPSI#issuecomment-514918345, or mute the thread https://github.com/notifications/unsubscribe-auth/AFMSG2KJPK77CVZICT3MBVDQBFDMFANCNFSM4IGAQFQQ .

sdeepaknarayanan commented 5 years ago

You can directly use the data from the link I suppose? You can add a mention in the notebook else. You have to add the find_closest.py script to utils. You haven’t I think. I get an import error when I run it. Add that commit and I’ll review once more. Check this: https://github.com/sustainability-lab/polire/tree/dev2/polire/utils

apoorvagnihotri commented 5 years ago

I'll update you with the link to data and the code soon. Thanks for bringing this up!

-- Apoorv Agnihotri Computer Science Department Final year Undergraduate IIT Gandhinagar apoorvagnihotri.github.io

Sent using Android.

On Thu, Jul 25, 2019, 12:58 PM S Deepak Narayanan notifications@github.com wrote:

You can directly use the data from the link I suppose? You can add a mention in the notebook else. You have to add the find_closest.py script to utils. You haven’t I think. I get an import error when I run it. Add that commit and I’ll review once more. Check this: https://github.com/sustainability-lab/polire/tree/dev2/polire/utils

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/sustainability-lab/polire/pull/18?email_source=notifications&email_token=AFMSG2NS2WNOFK2EEOLDRWDQBFI3DA5CNFSM4IGAQFQ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2YTRZA#issuecomment-514930916, or mute the thread https://github.com/notifications/unsubscribe-auth/AFMSG2JWJN7O7TRK6EI7H43QBFI3DANCNFSM4IGAQFQQ .

sdeepaknarayanan commented 5 years ago

I am merging it. I think the requested changes are minor and can be easily changed in future commits.