AequilibraE / aequilibrae

aequilibrae - Python package for transportation modeling
https://www.aequilibrae.com
Other
166 stars 38 forks source link

Route choice #531

Closed pedrocamargo closed 3 months ago

pedrocamargo commented 4 months ago

To be merged when all issues solved

Jake-Moss commented 4 months ago

@janzill This should now be fixed https://github.com/AequilibraE/aequilibrae/pull/532#issuecomment-2144114087. We're also now returning a column mask to indicate if the route was excluded because of the binary logit, not just setting it's probability to 0.

I plan to fix the disconnected ODs issue tomorrow morning. I think I have an idea of what is going on (and may have already partially fixed it)

janzill commented 4 months ago

@janzill This should now be fixed #532 (comment). We're also now returning a column mask to indicate if the route was excluded because of the binary logit, not just setting it's probability to 0.

I plan to fix the disconnected ODs issue tomorrow morning. I think I have an idea of what is going on (and may have already partially fixed it)

I tested this, it gives the correct results for path overlap, thanks Jake!

pedrocamargo commented 3 months ago

Feel free to merge if it all looks good, @janzill

Jake-Moss commented 3 months ago

I've reviewed most of this as part of the route choice docs branch that was just merged, this looks good. @Jake-Moss just double-checking - do we require pyarrow==15.0.0?

It's less of an actual dependency and more of a packaging issue. To get the wheel repair working on linux I had to specify the version of pyarrow. I'll give it another shot today after @pedrocamargo solved the macos build issues.

I've opened a discussion on the CIBuildWheels page asking for some general advice regarding this https://github.com/pypa/cibuildwheel/discussions/1872

Jake-Moss commented 3 months ago

I'm very unsure how this packaging should be handled. As I understand it the repair process involves finding external libs and placing them inside the wheel and correcting the paths. I've got a couple concerns with this,

janzill commented 3 months ago

Thanks @Jake-Moss in that case I am happy for this to be merged