-
Notifications
You must be signed in to change notification settings - Fork 6
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
first draft for implementing link vehicle capacity counts handler #189
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. Interesting concept.
Note that the transit capacities will be scaled in a pretty non-linear way, such that multiplying through by the scale factor might not make much sense. Nothing you can really do about this though.
If this becomes regularly used or project critical please add to readme and an example config.
btw, the post processing module could be used to automate further processing, eg flows/veh capacity.
Fixin
return veh_capacity | ||
else: | ||
return 5.0 | ||
|
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.
veh_type = self.resources['transit_vehicles'].veh_id_veh_type_map.get(vehicle_id, None)
return self.resources['transit_vehicles'].veh_type_capacity_map.get(veh_type, 5)
i would have written this as above, but on second thoughts, yours might actually be faster if it's predominantly cars so ignore.
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.
Hi Fred, thank you so much for pointing this out as well as your comment regarding scaling. We had some further discussion on this and decided that car capacities are too heavily affected by scaling and won't be too relevant unless if something like ridesharing is investigated. We thought we would focus more on pt and pt crowding at this stage so I think I might add something very similar to the link passenger counts
handler and make 'cars' an invalid mode.
""" | ||
super().__init__(config=config, mode=mode, groupby_person_attribute=groupby_person_attribute, **kwargs) | ||
|
||
self.groupby_person_attribute = groupby_person_attribute |
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.
i doubt this attribute groupby will ever be used? But also I think it doesn't really cause significant overhead so may aswell leave in
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.
ah, I was thinking about this as well, I wasn't sure whether or not it should be kept in there. I will look into simplifying / removing it in the next draft :)
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.
I think that there is not too much harm leaving it in, other than readability and maintainability.
I don't think there shoulld be any aweful consequences is removed other than a headache
standing_capacity = float(elem.xpath("capacity/standingRoom/@persons")[0]) | ||
else: | ||
seated_capacity = float(elem.xpath("capacity/@seats")[0]) # new structure | ||
standing_capacity = float(elem.xpath("capacity/@standingRoomInPersons")[0]) |
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 fine for now, it doesn't get called many times so speed not a concern. I will keep an eye on this format to see if it is the new standard, but meanwhwile happy this supports both.
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 a result of a breaking change between versions 1 and 2 of the vehicles definition schema, as described in my comment on the issue raised by @elizabethc-arup.
We should check the entire schema and make sure Elara supports both versions, not just for this element, but for anything that has changed.
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 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 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.
The feature looks good and it's great to see unit tests.
There are a bunch of files in the diff that seem like maybe they should be in .gitignore
- all of those binary files in tests/test_intermediate_data
for example. I cannot see them being used anywhere - are they just an unimportant byproduct of running the tests? If so, we shouldn't be version controlling them. Similarly we have some files in tests/test_outputs
and out/
- do these belong in the versioned repo? Some of them seem to have conflicts too:
Soon we will be open sourcing Elara, so now is a good time to tidy up.
tests/test_2_event_handlers.py
Outdated
# Car | ||
# subpopulation breakdown | ||
@pytest.fixture | ||
def test_car_capacity_count_handler_with_subpopulations(test_config, input_manager): |
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.
I don't think it's a good idea to name a fixture test_
, because that makes it look like a test, to both humans and the pytest test runner.
tests/test_2_event_handlers.py
Outdated
handler = test_car_capacity_count_handler_with_subpopulations | ||
for elem in events: | ||
handler.process_event(elem) | ||
assert np.sum(handler.counts) == 70 |
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.
Why do we expect it to be 70? Is this implicit knowledge about the fixture? Is there a way to be more explicit about our expectation?
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.
Hi Michael the 70 comes from the total capacity of the bus vehicle defined in the vehicles.xml file. I can't think of another way of embedding it to this test apart from importing and parsing the vehicles.xml, for now I have added a comment next to the assert stating where the number comes from, however, I am aware that this might not be the best practice. Would you be able to give me some advice on how I can best approach this? :(
tests/test_2_event_handlers.py
Outdated
handler.finalise() | ||
assert len(handler.result_dfs) == 2 | ||
for name, gdf in handler.result_dfs.items(): | ||
cols = list(range(handler.config.time_periods)) |
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.
Should cols
be defined outside of the loop?
tests/test_2_event_handlers.py
Outdated
assert c in gdf.columns | ||
assert 'total' in gdf.columns | ||
df = gdf.loc[:, cols] | ||
assert np.sum(df.values) == 14*5 / handler.config.scale_factor |
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.
Again, it's not clear why we have this expectation; 14 * 5
is a magic number here. Can we be clearer about why we expect what we expect?
tests/test_2_event_handlers.py
Outdated
|
||
# No class breakdown | ||
@pytest.fixture | ||
def test_car_capacity_count_handler_simple(test_config, input_manager): |
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.
Perhaps a better name would be something like car_link_vehicle_capacity_handler
? Or maybe even something more descriptive that describes how this handler is configured?
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.
thank you Michael, I have renamed all the test names to something that is a lot more descriptive as you suggested :)
3d9fe7f
to
cad47d0
Compare
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.
I've made some minor changes to a couple of unit tests to remove some of the magic numbers and make the tests easier to understand, but we could still do a little more along those lines.
standing_capacity = float(elem.xpath("capacity/standingRoom/@persons")[0]) | ||
else: | ||
seated_capacity = float(elem.xpath("capacity/@seats")[0]) # new structure | ||
standing_capacity = float(elem.xpath("capacity/@standingRoomInPersons")[0]) |
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.
…x errors in freshly built environments
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.
I added the missing tests, so I think we are probably ready to go now @elizabethc-arup.
standing_capacity = float(elem.xpath("capacity/standingRoom/@persons")[0]) | ||
else: | ||
seated_capacity = float(elem.xpath("capacity/@seats")[0]) # new structure | ||
standing_capacity = float(elem.xpath("capacity/@standingRoomInPersons")[0]) |
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.
@@ -1,12 +1,14 @@ | |||
click==7.0 | |||
colorama==0.4.4 | |||
Fiona==1.8.20 |
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.
I needed to add these pinned versions of a couple of transitive dependencies to make the unit test suite work in my local environment. May just have been something specific to my own setup, but I built the env from scratch and it was broken. Pinning to these versions hasn't broken the build, and it may well be a no-op for most people/environments.
@@ -503,3 +504,29 @@ def test_load_input_manager(test_xml_config, test_paths): | |||
assert input_workstation.resources['plans'] | |||
assert input_workstation.resources['input_plans'] | |||
assert input_workstation.resources['mode_map'] | |||
|
|||
|
|||
def test_parses_vehicle_capacity_from_file_using_vehicle_definitions_1_schema(): |
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.
These are the tests for the if/else
around the schema version in inputs.transform_veh_type_elem
.
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.
Hi Michael, thank you so much. Creating independent version 1 and version 2 xmls and tests make a lot of sense and is a lot clearer and easier to follow than what I have been trying to implement. I will keep this in mind when I implement my next set of pytests, thank you once again :D
This is linked to the link vehicle capacity feature request described in the issue #187, it adapts the link volume counts handler and adds vehicle capacities instead of counts so that an event like
<event time="25201.0" type="entered link" vehicle="bus1" link="2-3" />
will result in the bus vehicle capacity forbus1
added tolink 2-3
.For pt vehicles, the vehicle capacity is determined by mapping the vehicle ids to capacities using the functions in the
TransitVehicles
class in inputs.py and for cars, a default capacity value of5
is used.elara/elara/inputs.py
Lines 406 to 468 in 9a68ece
Currently, an
if statement
is used to deal with the transit_vehicle.xml version/legacy issue described in #188 as a temporary solution, it would be great if I could get some advice also on how best to resolve this. Thank you :)