search

BauhausLuftfahrt / MATSim-UAM

Extension for MATSim to allow for the inclusion of aerial passenger-carrying vehicles, i.e. a station- and network-bound transport mode that requires station access and egress trips with conventional ground-based modes.
https://bauhausluftfahrt.github.io/MATSim-UAM/
GNU General Public License v3.0
21 stars 28 forks source link

Dynamic search radius issue #67

Closed balacmi closed 3 years ago

balacmi commented 3 years ago

It seems to me that the dynamic search radius option is not used properly. The code here:

https://github.com/BauhausLuftfahrt/MATSim-UAM/blob/f25dd3ae4a476dc3bcd4e991455e31daa50106c3/src/main/java/net/bhl/matsim/uam/router/strategy/UAMStrategyUtils.java#L81

would make the search radius considerably large. Imagine a situation where the original search radius is 5000 meters and the Euclidian distance of the trip is just 100 meters. This would turn into a 500 km search radius.

I am nost sure what was even meant here to be able to fix it. At a minimum I would put the default of the: https://github.com/BauhausLuftfahrt/MATSim-UAM/blob/f25dd3ae4a476dc3bcd4e991455e31daa50106c3/src/main/java/net/bhl/matsim/uam/config/UAMConfigGroup.java#L54

to false

RRothfeld commented 3 years ago

I'll look into it today. But even without a fix, I agree that the default for dynamic should be false.

RRothfeld commented 3 years ago

Ah, I see. The idea is/was that if you use a dynamic search radius (i.e. useDynamicSearchRadius = true) that the searchRadius itself is understood as a ratio in percent. For example, the searchRadius would then be 0.3, meaning the search radius is 30% of the trip's Euclidean distance.

True, if someone uses dynamicSearchRadius and keeps 5000, the search radius is way too massive.

We could include a warning if the dynamicSearchRadius is true and the searchRadius is above 1. Any ideas?

balacmi commented 3 years ago

I was thinking later that that might be the way you intended it. I would put it to false, but provide a comment in the config group that explains exactly what you wrote above. So someone can use it that way, which makes sense and can also speed-up computation by reducing the search space.

Could you update the pull request and merge it?