Fix geometry and indexing of new/modified roadway nodes/links #310

e-lo · 2023-03-01T17:01:43Z

This PR contains major refactoring of several key pieces of RoadwayNetwork to be more efficient, resilient and understandable.

Improves:

Efficiency in apply managed lane, add/delete and modify property changes
Decomposes many monolithic functions and files (still many more to address)
Adds num_managed_lane_links() method to easily test for managed lanes ([Refactor] Separate out and be more rigorous about checking for managed lane network presence #294)
DRY principles
Efficiency in tests: uses small network when able to, uses fixtures for network so only need to read in once ([FEATURE] Use network fixture for testing #300)
Comprehensiveness in tests: actual assert statements, more tests
Organization of tests: into smaller files so that can easily run them and understand what each does
Organization of model-network functions into separate file as a model for decomposing some of roadway network which is...massive
added documentation at function level
formatting

Fixes:

How geometries are modified when nodes/links are added/deleted/modified and for managed lanes ([BUG] Nodes need to update their geometry if their XY changes. #297)
How indices are modified when nodes/links are added/deleted/modified and for managed lanes
A LOT of smaller bugs

...still a lot to do and that this PR doesn't fix.

- make easier to find various tests, utils and roadway network - troubleshooting test_model_roadway.py

Fixed: - indexing when created new links and nodes - merge that was creating excess records b/c it should have been an inner join Also: - added missing node from small ex network - made tests quicker by using small example network instead of large network - edited tests to be flexible to network tested - Made networks and directories test fixtures for simplicity

Because we are using separate files, don't need marks.

As separate from unique_model_link_identifiers so that we can use identifiers such as osm_link_id which may not be unique because of link splitting, but are sufficient for things like specifying links for project cards.

Fixed: - Querying if "set" or "existing" or "change" is a key, not if the value of it produces a True b/c a value of 0 is a valid "set"...but produces a False. Also: - Updated roadway change tests to actually test if change is accurate.

- To better manage and run them, tests for different apply methods are separated in different files - managed lane tests now have explicit assert statements testing actual truthiness Also: - convenience method added to RoadwayNetwork to count number of managed lane links

- Added RoadwayNetwork class variables for properties to coerce - Added class function to do coercion and call it at read-in and write out Also: - fixed bug in how test was looping

various minor bug fixes

- currently failing but not fixing as part of this PR

Includes: - added validation of new links and nodes - better handling of unique IDs - better structure for geometry creation/updating Also: - corrected occurrences of in/outboundReferenceId --> in/outboundReferenceId*s* - Added a simple roadway addition test

- fixed testing data to comply with roadway network requirements - added step to make sure new nodes had correct geometry - refactored how to delete nodes and links to be more efficient rather than searching through whole network for each selection - refactored how to search for shapes that should be deleted when links are - added better testing to make sure it was 'safe' to delete nodes Also: - black

e-lo · 2023-03-01T23:50:25Z

@lmz @DavidOry @i-am-sijia - This is passing the roadway tests on my machine. Def not perfect but better and more resilient.

i-am-sijia · 2023-03-07T21:31:12Z

tests\test_utils.py fails at test_point_from_xy():

After I change the output_crs to 4269, it passes. I think this is related to Issue #302

i-am-sijia

ran all tests locally, proposed changes necessary for tests to pass
the returns in transitnetwork.py seem inconsistent, after introducing net which is a deepcopy of self
the roadwaynetwork.py has tons of changes, which I have not yet detailly reviewed, but the tests all passed.
the SCRATCH_DIR in "test_transit.py" and "test_scenario.py" does not exist by default. It will be created after test_io.py is run which calls the pytest fixture. We need to be careful about the test dependencies when migrating to GitHub Actions.

network_wrangler/transitnetwork.py

i-am-sijia · 2023-03-07T22:16:55Z

network_wrangler/transitnetwork.py

-            return updated_network
+        q = net.feed.frequencies.trip_id.isin(freq["trip_id"])
+
+        net.loc[q, properties["property"]] = build_value


Suggested change

net.loc[q, properties["property"]] = build_value

net.feed.frequencies.loc[q, properties["property"]] = build_value

good catch!

i-am-sijia · 2023-03-07T22:19:02Z

tests/test_performance.py

+SHAPES = os.path.join(EX_DIR, "shape.geojson")
+
+
+def read_ex_net(benchmark):


Where is fixture benchmark defined?

Benchmark is a keyword for pytest benchmark for timing the tests.

tests/test_roadway/test_changes/test_pycode.py

i-am-sijia · 2023-03-07T23:01:05Z

tests/test_roadway/test_changes/test_roadway_feature_change.py

+    net = copy.deepcopy(small_net)
+    net.links_df["my_ad_hoc_field"] = 22.5
+
+    print("Network with field...\n ", net.links_df["my_ad_hoc_field"][0:5])


Suggested change

print("Network with field...\n ", net.links_df["my_ad_hoc_field"][0:5])

print("Network with field...\n ", net.links_df["my_ad_hoc_field"].iloc[0:5])

network_wrangler/utils/__init__.py

i-am-sijia · 2023-03-07T23:14:40Z

network_wrangler/transitnetwork.py

        """
+        net = copy.deepcopy(self)


Why do we need a deepcopy here? The same function in roadwaynetwork.py does not deepcopy.

network_wrangler/transitnetwork.py

i-am-sijia · 2023-03-07T23:23:44Z

network_wrangler/transitnetwork.py

+            net.feed.shapes = shapes
+            net.feed.stops = stops
+            net.feed.stop_times = stop_times
+            return net


The indentation of this return seems off, should we move it outside of the if?

i-am-sijia · 2023-03-07T23:32:09Z

examples/single/link.json

@@ -15,7 +15,7 @@
    "roadway": "residential",
    "length": 80.935,
    "maxspeed": "",
-    "name": "Arkwright Street,North Rivoli Street",
+    "name": "Arkwright Street",


Why shorten the name?

Just to differentiate from the larger network when I'm reading errors (this net is really not "real")

Co-authored-by: Sijia Wang <[email protected]>

…/network_wrangler into add-update-node-geometry

- did slightly different than suggestion so that would work if index value changed

- minor bug fixes

e-lo · 2023-03-08T23:53:42Z

@i-am-sijia I think this is ready for you to re-review

i-am-sijia

Just one small issue. Otherwise looks good.

network_wrangler/transitnetwork.py

Co-authored-by: Sijia Wang <[email protected]>

…into add-update-node-geometry

e-lo · 2023-03-09T22:46:07Z

📓 that most of the transit tests aren't passing yet and will be worked on in a separate PR

i-am-sijia · 2023-03-09T22:53:32Z

@e-lo I added a lot of transit related codes to MTC's network wrangler: https://github.com/BayAreaMetro/network_wrangler/tree/generic_agency/network_wrangler, so that it can actually apply real transit project cards. it might be useful to include those in the transit related PR.

e-lo · 2023-03-10T00:01:21Z

@i-am-sijia I created an issue where you can add more detail if you think it would be useful: #319

e-lo added 21 commits April 25, 2022 09:40

clean menow from testing

28ea40e

Add utility function for creating Point from xy

0bbfe65

test_point_from_xy tested and working

7959021

Refactor out reading of links/nodes/shapes to separate methods

5a4147a

Debugging

5dbd752

add functionality to update link and shape geometry as well as nodes

b85766a

change to use .update and update test to look at links

ccce3ae

test passing

60dd623

Split files / troubleshoot model network creation

f27b40f

- make easier to find various tests, utils and roadway network - troubleshooting test_model_roadway.py

Remove test marks

edefc3f

Because we are using separate files, don't need marks.

Make logger error reporting slightly more elegant.

e677038

Add EXPLICIT_LINK_IDENTIFIERS

f062e01

As separate from unique_model_link_identifiers so that we can use identifiers such as osm_link_id which may not be unique because of link splitting, but are sufficient for things like specifying links for project cards.

Debugged roadway change

3eff40f

Fixed: - Querying if "set" or "existing" or "change" is a key, not if the value of it produces a True b/c a value of 0 is a valid "set"...but produces a False. Also: - Updated roadway change tests to actually test if change is accurate.

Fixed types not coercing to strings to do search

b468f05

- Added RoadwayNetwork class variables for properties to coerce - Added class function to do coercion and call it at read-in and write out Also: - fixed bug in how test was looping

fix typo / update string formatting to use f-strings

7e20040

test_managed_lane.py now passing

90d7f4e

various minor bug fixes

added assert statements to pycode tests

c0ddb2f

- currently failing but not fixing as part of this PR

e-lo added bug Something isn't working enhancement New feature or request labels Mar 1, 2023

e-lo added this to the v1.0 milestone Mar 1, 2023

e-lo requested review from lmz, DavidOry and i-am-sijia March 1, 2023 17:01

e-lo self-assigned this Mar 1, 2023

e-lo changed the base branch from master to develop March 1, 2023 17:07

e-lo marked this pull request as draft March 1, 2023 17:07

e-lo marked this pull request as ready for review March 1, 2023 23:49

e-lo mentioned this pull request Mar 1, 2023

Create pytest fixtures #301

Closed

This was linked to issues Mar 2, 2023

[BUG] Nodes need to update their geometry if their XY changes. #297

Closed

[Refactor] Separate out and be more rigorous about checking for managed lane network presence #294

Closed

e-lo mentioned this pull request Mar 3, 2023

Address MTC Logging Concerns #313

Merged

i-am-sijia requested changes Mar 7, 2023

View reviewed changes

e-lo and others added 3 commits March 8, 2023 09:59

Update network_wrangler/utils/__init__.py

0ca74f6

Co-authored-by: Sijia Wang <[email protected]>

suggested changes from sijia

f8aa826

Merge branch 'add-update-node-geometry' of https://github.com/wsp-sag…

f961544

…/network_wrangler into add-update-node-geometry

e-lo requested a review from i-am-sijia March 8, 2023 18:01

e-lo added 2 commits March 8, 2023 15:34

Fix failing pycode test

d662999

- did slightly different than suggestion so that would work if index value changed

passing tests again

684ccfe

- minor bug fixes

i-am-sijia requested changes Mar 9, 2023

View reviewed changes

network_wrangler/transitnetwork.py Show resolved Hide resolved

Update network_wrangler/transitnetwork.py

cd89390

Co-authored-by: Sijia Wang <[email protected]>

e-lo requested a review from i-am-sijia March 9, 2023 17:46

i-am-sijia approved these changes Mar 9, 2023

View reviewed changes

e-lo added 2 commits March 9, 2023 14:37

Merge branch 'develop' of https://github.com/wsp-sag/network_wrangler …

606de96

…into add-update-node-geometry

address old var to pass flaking

60c641a

e-lo merged commit 5d87f2f into develop Mar 9, 2023

e-lo deleted the add-update-node-geometry branch March 9, 2023 23:40

This was referenced Mar 14, 2023

[FEATURE] Use network fixture for testing #300

Closed

[BUG] Nodes need to update their geometry if their XY changes. #297

Closed

offset_lat_lon is not a defined method or variable. #257

Closed

write assert for test_managed_lane_project_card #246

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix geometry and indexing of new/modified roadway nodes/links #310

Fix geometry and indexing of new/modified roadway nodes/links #310

e-lo commented Mar 1, 2023

e-lo commented Mar 1, 2023

i-am-sijia commented Mar 7, 2023

i-am-sijia left a comment

i-am-sijia Mar 7, 2023

e-lo Mar 8, 2023

i-am-sijia Mar 7, 2023

e-lo Mar 8, 2023 •

edited

Loading

i-am-sijia Mar 7, 2023

i-am-sijia Mar 7, 2023

i-am-sijia Mar 7, 2023

e-lo Mar 8, 2023

i-am-sijia Mar 7, 2023

e-lo Mar 8, 2023

e-lo commented Mar 8, 2023

i-am-sijia left a comment

e-lo commented Mar 9, 2023

i-am-sijia commented Mar 9, 2023

e-lo commented Mar 10, 2023

	net.loc[q, properties["property"]] = build_value
	net.feed.frequencies.loc[q, properties["property"]] = build_value

		SHAPES = os.path.join(EX_DIR, "shape.geojson")


		def read_ex_net(benchmark):

	print("Network with field...\n ", net.links_df["my_ad_hoc_field"][0:5])
	print("Network with field...\n ", net.links_df["my_ad_hoc_field"].iloc[0:5])

Fix geometry and indexing of new/modified roadway nodes/links #310

Fix geometry and indexing of new/modified roadway nodes/links #310

Conversation

e-lo commented Mar 1, 2023

e-lo commented Mar 1, 2023

i-am-sijia commented Mar 7, 2023

i-am-sijia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

e-lo Mar 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

e-lo commented Mar 8, 2023

i-am-sijia left a comment

Choose a reason for hiding this comment

e-lo commented Mar 9, 2023

i-am-sijia commented Mar 9, 2023

e-lo commented Mar 10, 2023

e-lo Mar 8, 2023 •

edited

Loading