traffordDataLab / leaflet.reachability

Plugin for the Leaflet JavaScript library to show areas of reachability based on time or distance for different modes of travel using the openrouteservice isochrones API.
MIT License
64 stars 7 forks source link

v1.0.0 unexpected results if range interval value does not equal minimum range value #5

Closed itsozz closed 5 years ago

itsozz commented 5 years ago

This is caused due to a misunderstanding on how the intervals work. The following example will hopefully make this clearer. Note: this applies for time and distance.

Example: Range minimum = 500 m Range maximum = 1000 m Range interval = 100 m

The range selection list is populated as follows: 500 m 600 m 700 m 800 m 900 m 1000 m

If you request a range of 600m with intervals you would expect 2 isochrones to be created, the first at 500m and the second at 600m. However the result is 6 isochrones, starting at 100m up to 600m. This is due to the fact that there is only ever one range value passed to the API, regardless of whether the user wants intervals or not. If they want intervals, this is passed to the API using the relevant parameter. If the interval value matches the minimum range value then the correct result occurs, e.g. a range of 0.5-3km with an interval of 0.5km.

nilsnolde commented 5 years ago

FYI, we had a request before for minimum_range, but decided against it, since you can just specify range with a list of arbitrary distance/time values. In case you weren't aware (we actually weren't ourselves until a year ago or so;)).

itsozz commented 5 years ago

Thanks Nils, really appreciate your feedback. You are correct, I wasn't aware until today that I could specify range as an array of values! My proposed fix will pass an array for range with either a single value or for intervals, multiple values from the minimum range option up to the range value chosen. Still testing now but this seems to work.

The reason for needing a minimum, maximum and interval for the plugin is due to attempting to represent effectively 2 sliders, (one for the range and the other for the interval) within a simple select list. Perhaps it would be better to allow the user to supply the range values as an array, rather than calculate them - however this would be a breaking change and so necessitate a major version increase! Initially I'm planning to release a patch update to fix the bugs without breaking the current operation, however I'd be interested to hear your thoughts on this.

nilsnolde commented 5 years ago

Can you remind me again where I can find the online resource? It's not this one, right?

itsozz commented 5 years ago

The plugin is implemented on our Explore application that you've linked to and there is also a demo within this repo. Is that what you were looking for? I'm currently making changes locally regarding this issue and the others I've opened but will be pushing the updates very soon.

nilsnolde commented 5 years ago

Yeah, within the plugin it's probably pointless to work with arbitrary values. Did you look at our frontend implementation? I guess so..;) I can put you in contact with our frontend dev.

You could also do it with the arbitrary range list in your code. The interval slider updates with the range slider and the interval slider only allows range/10 (integer) values. You take the values from the interval slider and put them in the range object of the isochrone endpoint.

Btw, I'd strongly recommend you move to v2 endpoints, i.e. POST: https://openrouteservice.org/dev/#/api-docs/v2/isochrones/{profile}/post

Any API bug fixes won't be going into the old GET endpoints anymore.

itsozz commented 5 years ago

Thanks Nils. Yes I used your frontend implementation as the basis for the plugin and then had to simplify everything in order for it to be as compact as possible. The trade-off of course is it loses some of the functionality.

Thanks for the suggestions, I'll bear them in mind for future releases. I've been having a look at the V2 documentation and plan to upgrade the plugin to use this. However for the next release I just wanted to fix some bugs without breaking any existing implementations. Will the old GET endpoints be staying active or do you have plans to shut them down? I'll be closing this issue shortly but please feel free to continue to add comments.

nilsnolde commented 5 years ago

Yeah, our frontend code is a bit of a mess;)

Will the old GET endpoints be staying active or do you have plans to shut them down?

Not anytime soon, by the end of the year. But we're working a little on isochrones right now, the geometry (has to get more accurate, not so generalized) and the algorithmic side (longer ranges >> 1 hour, rather 4-5 hours). Not sure if all that would be available for v1. But that's still a few months to go as well. If you follow us on Twitter, you'll get notified though:)

itsozz commented 5 years ago

We certainly do follow you on Twitter, and thanks for the heads-up regarding the old endpoints - will have to prioritise upgrading to V2!

itsozz commented 5 years ago

Having discussed this further it seems that the original behaviour is the correct and expected one and perhaps it is the minimum value for the select list that is the confusing aspect here. Therefore I'm closing this and not making any changes. However the discussion above will guide the implementation in the version which uses the V2 API. Thanks again Nils for your input on this, really appreciated.