-
Notifications
You must be signed in to change notification settings - Fork 10
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
Convenience method to strip links of mode #246
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I have asked a question prompted by one of the new tests, but it's not about anything introduced in this PR.
def test_removing_mode_from_all_links_deletes_links_with_no_other_modes(): | ||
n = Network("epsg:27700") | ||
n.add_link("0", 1, 2, attribs={"modes": {"car", "bike"}, "length": 1}) | ||
n.add_link("1", 2, 3, attribs={"modes": {"bike"}, "length": 1}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not directly about this PR, but this test raises a question: Is there some warning in the logs or elsewhere when you remove all modes from a link and that link is subsequently removed from the graph? I get that a link with no modes cannot be traversed, and so can be removed, but the user might have done that inadvertently, and the downstream effects could be very confusing if so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are logs when links are removed. The removal of links is not automatic, unless you use these dedicated methods. So a user could mess around with modes on the links and the structure of the graph remains the same. In either case, whether the links are removed or not (so, just modes missing from links) genet has methods to verify validity of the graph and if they pose issues these will come out in the reports. These run automatically if using genet cli commands, or can/should be called upon if a user is using genet as a package and expecting to use their network with matsim.
Closes #243 , first in a series of 3, to facilitate better active mode support
I lowered code coverage - I have no idea why my additions would have lowered it, I added only a couple of lines and tested them... To bring it up I would have to up the code coverage for other part of the code that are not relevant to this PR...
The overall changes to code are small, the PR got inflated a bit with a cruft update and formatting of some of the jupyter notebooks.
I also removed Windows from the build, it doesn't look like this issue will get solved very soon. We can add it back in after the problem is fixed.