-
Notifications
You must be signed in to change notification settings - Fork 16
Conversation
Used the haversince formula instead of |
gwr/gwr.py
Outdated
@@ -121,6 +125,10 @@ class GWR(GLM): | |||
constant : boolean | |||
True to include intercept (default) in model and False to exclude | |||
intercept | |||
|
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 says boolean, but at the moment it is a string. Is it perhaps more straight-forward to change coords_type = string
to something like spherical=False
which makes the default projected coordinates but can be changed to spherical=True
to use lat/long, the haversine formula, etc.
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.
I previously used True/False for that but switched to use a more explicit string for coords_type
and forgot to change the comment. I think coords_type = "longlat"
helps user to know the coordinates are in the order of long-lat not lat-long.
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.
I'm still thinking it would be more straight-forward to use a true boolean here, since there are only two appropriate options. Could make a note under the docs for coords
that highlights that it takes a list of tuples of (x,y) or (long, lat) coordinates.
For instance, using a string argument makes it easier for user to input an incorrect input. If anything but "projected" or "longlat" is input then cdist will return a ``dmat`' of all zeros. You could add a try/except block, but whatever error you raise will be given regardless of whether they user has used an incorrect input or there is some other error while computing distances...say np.inf values are present for some reason. This is the issue we have now with the try/except block around building W with different types of kernels, but can be avoided here since we there really are only two options (spherical/projected).
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.
That makes sense, I will change to T/F very soon.
😃 Changed coords_type to spherical=T/F and waiting for the tests. |
Related to #6
Ship the feature allows for longlat coords. Added a test from GWR4, and they match. All functions/classes that need a
coords
arg also needs acoords_type
arg indicating whether it is projected or longlat.