-
Notifications
You must be signed in to change notification settings - Fork 63
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Isochrones & Isodistances #957
Conversation
Problems so far:
TODO:
|
# Conflicts: # cartoframes/analysis/__init__.py
Hi! 👋 I've a question regarding the API naming: We're importing the 'Isolines' class, and then we're creating from this instance an Isochrone. That's because the Isoline can be an Isochrone or an Isodistance instance. Would be possible to have a common Isoline Class, and then two classes (Isochrone and Isodistance) that inherit from this class? So we can do something like: from cartoframes.services.isolines import Isochrone, Isodistance
isochrone = Isochrone(dataset, [100, 1000], mode='car')
isodistance = Isodistance(dataset, [100, 1000], mode='car') This is just a suggestion, I don't know what's the correct way. I'm just thinking about using the same convention we're using in the rest of the classes. What do you think? |
hi! I have some questions too!
|
I'm glad you brought out the topic of the naming, cause I'm not happy either with the current isolines (which I used because we use it in the docs. I used |
@mamata: for the API follows closely the SQL (data services) interface. So you specify an array of range values (times for isochrones/distances for isodistances) and get one boundary for each value and input point which encloses all the area within (less than) that time/distance (so we don't generate rings, but simple polygons). For example here we have two input points (A, B, green) and two range values, 5 and 15 and we get four resulting isochrone polygons without holes: We have a somewhat higher level interface in camshaft (builder analyses for trade areas, buffers). The difference is that the number of ranges and max range is passed instead of individual ranges values (so ranges are max/n, ..., max) and that there's a dissolved option where all the isochrones for each range are merged into one (instead of having separate isochrones for each center point). In the previous example we would have two multi polygons, one with boundaries A-5, B-5 and other with A-15, B-15.
Now, when having multiple ranges we have the problem of larger range value polygons hiding the smaller values, since the larger value ones always completely cover all the smaller ones. As you have noticed this is going to be a real problem for the users. What can we do about it?
The problem with the two first is that I don't see how we could automate that styling, and I think that just advising the users how to style isochrones result wouldn't be enough. I like the idea of generating rings, but that can be computationally expensive, and not always what the user needs. We could add options for that, but we'd need to make rings the default to avoid the visualization problem for unsuspecting users. And I also would like to generate separate datasets for each range value, but I'm afraid the resulting API would be too complex, what do you think?
I don't see how we could handle on the API part, since in the end the provider and quality inconsequence will depend on how much 💰 the user pays. |
🤔 To solve the overlapping ranges problem maybe we could have a helper method to add the ischrones result to a map which could add a layer for each range (filtering the result dataset by range value) |
Regarding the naming, yes, I like @elenatorro's suggestion as well and whatever you feel is the best class name. Obviously aligning with current doc makes sense but even in the doc, we say Isoline but then talk about generating polygons... I think that is the most confusing part for me! For example, the first part in the doc says lines, then polygon, then area..
Ahhhh, ok, I was, in my head, thinking that this was more similar to the way that we do it in Builder. Before I write more, let me try out this implementation to "see" what the result is to understand what output we'll get vs. the Builder one that I am familiar with and have in my mind. Then I'll be able to comment better on the other pieces. thanks!! |
@jgoizueta as discussed today, we can share the rationale for proposed naming with the group on Monday and also I will work on some viz ideas for the default output and we can discuss the visualization challenge with the group as well |
While completing the docs, I've notice a couple of things:
|
Also, cartodb_id is removed from output if it wasn't in the source
# Conflicts: # cartoframes/data/dataset/registry/dataframe_dataset.py
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 have added some comments about credentails-Dataset and a possible error. I am not sure if we should avoid doing lines longer than 120 even in comments.
In any case, it looks awesome.
super(Isolines, self).__init__(credentials, quota_service=QUOTA_SERVICE) | ||
|
||
def isochrones(self, source, range, **args): | ||
"""isochrone areas |
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.
Cool doc 💪
Credentials have already been set in the Dataset constructor
Dataframe datasets ignore the constructor credentials, and, in any case, for dataframes we're setting the Geocoder credentials to upload them.
Generate proper legend
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.
🚀 💪
Fixes #889
Interface:
There are two methods,
isochrones
andisodistances
; both take a dataset argument, an array of range values (seconds for isochrones, meters of isodistances) and a number of optional arguments.