Add cli command for attaching modal subgraphs #250
Conversation
There was a problem hiding this comment.
I've made a few inline comments and asked a few questions that represent fairly trivial things.
I've also described a cleaner separation of concerns, where the CLI code would only be doing CLI things. This is a bigger change and does not need to happen in this PR, but it's something to bear in mind. Right now, the CLI and the - for want of a better phrase - "domain API" are smudged together when it would make more sense to decouple them.
| """Add extracted modal subgraphs from the `subgraph network` to the main `network` (without merging links). | ||
|
|
||
| This creates separate modal subgraphs for the given modes. | ||
| It replaces any mentions or links of specified modes, in the original network, and replaces them with links taken |
There was a problem hiding this comment.
You can delete the and replaces them bit
| [1] subgraph_network.link("SUBNET_LINK_ID") | ||
| [out] {"id": "SUBNET_LINK_ID", "modes": {"car", "bike"}, "some_attrib": "sub_network", ...} | ||
| ``` | ||
|
|
There was a problem hiding this comment.
Can we put something in between the before state and after state of the two graphs that shows the actual API call that performs the transformation?
Like:
- Show the "before" state
- Do the thing
- Show the effect of the thing via the "after" state
| for mode in modes: | ||
| link_id_prefix = f"{mode}---" | ||
|
|
||
| logging.info(f"Cleansing original network from mode: {mode}") |
| path_to_subgraph: Path, | ||
| modes: str, | ||
| increase_capacity: bool, | ||
| ) -> Network: |
There was a problem hiding this comment.
A lot is going on in this function. Could it be refactored to make it more readable and easier to test?
| @@ -213,6 +215,23 @@ def test_separate_modes_in_network(self, invoke_runner_and_check_files): | |||
| ], | |||
| ) | |||
|
|
|||
| def test_replace_modal_subgraph(self, invoke_runner_and_check_files): | |||
There was a problem hiding this comment.
The test name doesn't give any clue as to what we expect to happen, but it could do if it were more like test_replacing_modal_subgraph_generates_expected_outputs or whatever.
|
|
||
| output_data = output_network.link(sub_network["expected_bike_link_id"]) | ||
| keys_to_ignore = {"from", "s2_from", "to", "s2_to", "id", "modes", "capacity"} | ||
| assert {k: v for k, v in output_data.items() if k not in keys_to_ignore} == { |
There was a problem hiding this comment.
Maybe this assertion can be extracted into a helper method like assert_link_data_equal(link_1, link_2) that would keep the intent of the assertion clear but remove some of the noisy detail of the comparison mechanics and make the test a little easier to understand.
| ), f"Link {link_id} did not retain th original capacity value" | ||
|
|
||
|
|
||
| def test_does_not_increase_capacity_if_not_requested(tmpdir, network, sub_network): |
There was a problem hiding this comment.
test_preserves_link_capacity_value_when_replacing_modal_subgraphs or suchlike, maybe?
| increase_capacity=False, | ||
| ) | ||
|
|
||
| for link_id, old_capacity in original_subnet_capacity.items(): |
There was a problem hiding this comment.
Again, it might help to extract the comparison into a helper function with a name that makes what we're asserting obvious but hides the details of how we perform that comparison.
| ), f"Link {expected_new_link_id} did not retain th original capacity value" | ||
|
|
||
|
|
||
| def test_adds_nodes_only_once_with_multiple_subgraphs(tmpdir, network, sub_network): |
There was a problem hiding this comment.
Technically, the same node could have been added multiple times, and this test would erroneously pass as long as the node count was as expected. If that is what we care about verifying, the test name should make that clear.
But if the test name accurately describes what we want to verify, the assertions should align with that intent. Right now, there is a disconnect between the intent of the test as suggested by the name, and the actual verification happening inside the test.
| increase_capacity=False, | ||
| ) | ||
|
|
||
| assert len(list(output_network.links())) == original_net_no_links + ( |
There was a problem hiding this comment.
I can't understand from this test why we expect twice the number of links we see in the sub-network. Is there a way to make that clearer?
Closes #245
This ends my 4-part series to enable attaching a modal subgraph to an existing network.
This cli command has been tested on a live project and smoke tested with MATSim. Steps and configs for batch jobs are available here.
Bonus: fixed a couple of warnings, comments inline