search

UDST / pandana

Pandas Network Analysis by UrbanSim: fast accessibility metrics and shortest paths, using contraction hierarchies :world_map:
http://udst.github.io/pandana
GNU Affero General Public License v3.0
386 stars 84 forks source link

Optimized std::string comparisons #111

Closed federicofernandez closed 5 years ago

federicofernandez commented 5 years ago

Part of the modifications from 0.3 to 0.4 included modifying the decay parameter from an enum to std::string. That is a good modification in terms of functionality but it seems to be that it made the library slower.

In this PR the comparisons are avoided by choosing a decay function before executing the loops. In my local environment, the performance improved (for the SEMCOG example) and it's very similar to v0.3.

coveralls commented 5 years ago

Coverage Status

Coverage remained the same at 91.089% when pulling 9d9e0a0dcd6c7e0b560b80022139332a91446ad5 on enhancement/optimize-string-comparisons into 68c2644ecd3e0095d64b7c493f12820e3e5c66d0 on develop.

Eh2406 commented 5 years ago

Sorry if I get the terminology wrong; I do not speak C++. Is this solution using dynamic dispatch? Does it make sense to move the inner loop into a separate function so that it can monomorphiz and inline the decay function?

federicofernandez commented 5 years ago

Sorry if I get the terminology wrong; I do not speak C++. Is this solution using dynamic dispatch? Does it make sense to move the inner loop into a separate function so that it can monomorphiz and inline the decay function?

No, it won't be dynamic. The lambda function will be defined before the loop begins, so it is expected that the compiler will inline it in the inner loop.

Moving the inner loop into a separate function doesn't look beneficial for performance, but it's highly probable that the compiler will also inline it.

smmaurer commented 5 years ago

FYI, PR #112 fixed the problem that's causing this PR's Travis failures. If you merge develop into this branch, the tests should pass.

federicofernandez commented 5 years ago

FYI, PR #112 fixed the problem that's causing this PR's Travis failures. If you merge develop into this branch, the tests should pass.

Thanks @smmaurer! I did that and now tests are passing.

federicofernandez commented 5 years ago

Just to be clear, this optimization should mainly impact the aggregate() method of the Python API (https://github.com/UDST/pandana/blob/master/pandana/network.py#L274).