Skip to content
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

Remove 999999 replacements for time values #367

Merged
merged 2 commits into from
Sep 1, 2021
Merged

Conversation

zptro
Copy link
Collaborator

@zptro zptro commented Sep 1, 2021

The code has looped through all OD-impedances (including travel time) and replaced them by 999999 if travel time is greater than 999999 (i.e., no path was found). In python 2.7 this worked fine, because travel time was usually last in the dict, so that replacement happened last. In python 3.7 however, travel time is usually first in the dict (because it was inserted first), so when handling the other impedances, travel time would no longer be greater than 999999!

I see not reason to replace travel time by 999999, as it will be a very large number anyway.

@zptro zptro requested a review from johpiip September 1, 2021 08:57
@zptro zptro added the bug Something isn't working label Sep 1, 2021
johpiip
johpiip previously approved these changes Sep 1, 2021
Copy link
Contributor

@johpiip johpiip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am approving but I do not really understand the PR message versus the changeset. 😕 As far as I see, the changeset contains a) looping explicitly through "cost" and "dist" matrices in that order, and b) defining "path not found" as greater than or equal to 999999 instead of greater than 999999. Do you mean that if "cost" and "dist" matrices change, also "travel time" matrix changes consequently?

@zptro
Copy link
Collaborator Author

zptro commented Sep 1, 2021

The change from > to >= was actually a mistake, good that you noticed! 😄

Hence, (a) is the point of this PR. Time matrices should not be looped through, as that messes up what we are trying to achieve, at least in python 3.7.

@zptro zptro requested a review from johpiip September 1, 2021 10:28
@zptro
Copy link
Collaborator Author

zptro commented Sep 1, 2021

Changing > to >= was an alternative way of solving this that I had in mind. But equality checks are unstable for floats, so I ditched that idea.

Copy link
Contributor

@johpiip johpiip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I get it, thanks! :)

@zptro zptro merged commit ea8a83d into olusanya Sep 1, 2021
@zptro zptro deleted the hotfix/bike_dist branch September 1, 2021 11:21
@zptro zptro added this to the v4.1 milestone Sep 1, 2021
johpiip added a commit that referenced this pull request Feb 3, 2022
* Car assignment last in end assigment (#352)

* Fix minor bug where buses in background traffic was sometimes for wrong
time period in iteration 1

* Do bike assignment before car assignment in last iteration
Need to explicitly set car vdfs after bike assignment

* Fix pandas indexing (#353)

* Enable workforce accessibility calculation and printing (#355)

* Merge README (#356)

* Fix `WindowsError` (#343)

* Fix `WindowsError`

`subprocess.CalledProcessError` only works if git client is installed

* Update version number

* Update README.md (#354)

* Update README.md

* Print line optimization (#359)

* Speed up line prints when printing many lines

* Re-add printing of the first line

* Remove transit passengers and bikes from vehicle kms (#364)

* Remove 999999 replacements for time values (#367)

* Remove truncation of time values

* Fix bus kms (#366)

* Remove rail vehicles from link attribute `@bus`

* Add vdf 0 to result prints

* Remove rail vehicles from link attribute `@bus` (really)

* Add comment

* Fix congested transit assignment connectivity issue in test_assignment.py (#370)

by adding bus stop

* Print KPI summary (#357)

* Add mode share summary

* Add accessbility indicators

* Add vehicle kilometres

* Add printing of assigned demand per time period

* Add whole model area to area workforce aggregation print

* Add workplace effective density calculation and printing

* Move aggregation and change space to tab

* Use home-work model for effective density calculation

* Update helmet version number

* Convergence indicators (#371)

* Add demand model convergence monitoring

* Change list into DataFrame and add documentation

* Update savu intervals to match new accessibility model (#372)

* Unify connector handling (#374)

* Insert node result aggregation into loop (#373)

* Hotfix/headways (#376)

* Add writing of headway attribute
when beginning congested transit assignment

* Fix congested assignment test

* Add testing of end assignment

* Update version number

* Run parameters (#377)

* Add convergence criteria as alternative to fixed number of iterations

* Make relative gap absolute number

* Refactor config (#383)

* Refactor config

- Remove unnecessary properties
- Remove unnecessary lookup from environment variables
- Restructure booleans into flags as for helmet args
- Change log parameter set from config to args

* Change SET_TRUE into OPTIONAL_FLAGS

* Fix json syntax

* Add missing log args in cba

These should not be activated by default, as they override cli parameters

* Add separate flag for saving results in separate EMME scenarios

* Implement END_ASSIGNMENT_ONLY parameter

* Aux transit volumes (#384)

* Add `aux_transit_volume` attribute and extra attribute name check

* Add saving and printing of `aux_transit_volume` attribute

* Add volume factor

* Clip base year car density at 1.0 (#388)

Higher values should not affect predictions.

* Add calculation of SAVU for population average (#387)

* Add calculation of SAVU for population average

SAVU is derived from logsum from sustainable transport modes,
according to seven fixed logsum intervals.
This addition checks what interval population averaged logsum is in.
Additionally, it check how far it is from the next interval,
hence adding decimals to the otherwise integer value.

* Change `travel_modes` from set to dict to preserve order. (#389)

* Print zone-specific CBA (consumer) results to text file (#394)

* Print zone-specific CBA (consumer) results to text file
- travel time change
- travel cost change
- distance change
- revenue change

* Update docstrings

* Add additional gains

* Add in-vehicle penalty for 999 min headway lines (#393)

* Add boarding penalty for 999 min headway lines
Refactor standard attribute handling

* Change line-specific boarding cost into in-vehicle cost.

Global boarding cost cannot be combined with line-specific.

* Add last iteration when reaching gap threshold (#396)

Co-authored-by: Johanna Piipponen <johanna.piipponen@wsp.com>

* Update dev-config.json (#398)

* Update README.md (#405)

* Fix bike mode and refactor mode handling (#412)

* Update data to match test network (#407)

* Update data to match test network

* Update impedance matrices to match new zone system

* Update tests and test data

* Make assignment results that can be used for demand calculation

* Allow cars on motorway ramps

* Update impedance matrices

* Update tests

* Fix `separate_emme_scenarios` argument in extra attribute validation

* Update version number

* Change bike mode into `param.main_mode` (#415)

`param.bike_mode` f is not an auto mode so it cannot be used directly.

* Add node validation (#411)

* Add node validation

* Add lightrail nodes

* Add upper limit to interval

* Change baseline zonedata folder name (#413)

* Hotfix/cba (#420)

* Fix zone-wise cba results

* Fix maintenance cost calculation

* Remove area dummies from accessibility indicators (#422)

* Remove area dummies from accessibility indicators

* Update savu intervals

* Fix git tag (#423)

* Change directory

* Direct error messages to pipe

* Create own print method for `SecDestPurpose` (#424)

Calculating mode shares does not work for secondary destinations,
because they do not have own generated demand, so `Purpose`
print method causes divide-by-zero warning.

* Add python version check (#425)

* Fix node number validation (#426)

* Add mode h to node check (#429)

* Add mode h to node check

* Add s mode to rail nodes

* Add KELA codes and handle them which lack zone aggregation (#431)

Co-authored-by: Jens West <jens.west@hsl.fi>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants