Promote PtNetworkSimplifier to org.matsim package

[marecabo]

With this PR, I'd suggest to move the PtNetworkSimplifier to the org.matsim package.

Recently, I used the PtNetworkSimplifier from the vsp playground a lot and it worked well as expected. Credits and many thanks to aneumann (is it @neuma?) for it.

I changed and added a few things:

• Made the API more compatible to the one of the regular NetworkSimplifier
• Parallelized some checks for improved performance on large pt networks.
• Wrote a test case

[Janekdererste]

Thanks for your suggestion. @vsp-gleich do you have an oppinion on this? Otherwise this looks fine for merging to me.

[vsp-gleich]

This class can be useful for models in which pt is sharing the network with car and other modes. Usually we have pt on a separate pseudo network for which no simplification should be possible. So, this is for now not the most essential functionality which must reside in the matsim core. But I'm not really against moving it either.
It would be nice to test the transit schedule modifications in the test and maybe test run any of the typical pt pseudo networks (e.g. from any open scenario) and see whether possible simplifications are reported/made (in my intuition there shouldn't be any simplification possible).
In comparison to the standard NetworkSimplifier this PtNetworkSimplifier only cares about NetworkTopoType and whether links with pt services are affected. Maybe some kind of delegation to the standard NetworkSimplifier would be better on the long run? The PtNetworkSimplifier only adds the check for pt services feature and the modification of affected pt services. What do you think @Janekdererste ?

[marecabo]

Background info: I used it actually to create pseudo-pt networks from networks generated with PT2MATSim. There, pt routes are mapped to OSM ways (while keeping their link geometries) and simplification was really helpful. If you do not need your pseudo-pt network to have real link geometries, then you can just create the pseudo-pt network from the schedule alone and don't need simplification.

[vsp-gleich]

Background info: I used it actually to create pseudo-pt networks from networks generated with PT2MATSim. There, pt routes are mapped to OSM ways (while keeping their link geometries) and simplification was really helpful. If you do not need your pseudo-pt network to have real link geometries, then you can just create the pseudo-pt network from the schedule alone and don't need simplification.

That makes sense, thank you for explaining! Actually when using pt2matsim to map stops to osm ways wouldn't it be helpful to then have the same simplification functions as the standard NetworkSimplifier? Especially if buses shall share the normal car network (since they are already mapped to it and road congestion effects are nice to have)?

[Janekdererste]

What do you think @Janekdererste ?

I would not automatically call the network simplifier from here. If someone is interestend in doing both, I would think it is better to do so explicitly. I am not sure whether I completely understood your suggestion though 🙃.

[vsp-gleich]

Thank you for adding a test for the modified TransitRoute!
The vsp contrib version of PtNetworkSimplifier should be deleted during the move.
EDIT: Remove phrase on apparently deleted TransitScheduleCleaner reference.

[vsp-gleich]

What do you think @Janekdererste ?

I would not automatically call the network simplifier from here. If someone is interestend in doing both, I would think it is better to do so explicitly. I am not sure whether I completely understood your suggestion though upside_down_face.

NetworkSimplifier and PtNetworkSimplifier have quite some overlaps (e.g. method bothLinksHaveSameLinkStats() and parts of the run() method), but neither can fully replace the other. This opens questions for code maintainability and end users. I fear some users will not be aware of both versions and their limitations, and will e.g. use NetworkSimplifier without thinking about consequences for pt services or re-implement features in PtNetworkSimplifier that already exist in NetworkSimplifier (e.g. more constraint/filter options to chose which links can be merged). That's why maybe merging the PtNetwork stuff into NetworkSimplifier might be better.

[Janekdererste]

That's why maybe merging the PtNetwork stuff into NetworkSimplifier might be better.

Now, that you've explained this to me offline, I do agree with your preference to merge the pt-simplifier into the regurlar NetworkSimplifier.

@marecabo would you be willing to do that instead?

[marecabo]

@vsp-gleich Thanks for the discussion last week. When I find some time to merge both, can I get rid of the these parts from the NetworkSimplifier, as they run method allowing to vary thresholdLength is already marked as deprecated?

• matsim-libs/matsim/src/main/java/org/matsim/core/network/algorithms/NetworkSimplifier.java

  Lines 101 to 116 in 2c5cd05

  	/**
  	* Merges all qualifying links while ensuring no link is shorter than the
  	* given threshold.
  	* <br/>
  	* Comments:<ul>
  	* <li>I would argue against setting the thresholdLength to anything different from POSITIVE_INFINITY, since
  	* the result of the method depends on the sequence in which the algorithm goes through the nodes. </li>
  	* </ul>
  	* @param network
  	* @param thresholdLength
  	*/
  	@Deprecated
  	public void run(final Network network, double thresholdLength){
  	run(network, thresholdLength, ThresholdExceeded.BOTH);
  	run(network, thresholdLength, ThresholdExceeded.EITHER);
  	}

• matsim-libs/matsim/src/main/java/org/matsim/core/network/algorithms/NetworkSimplifier.java

  Lines 161 to 181 in 2c5cd05

  	// Only merge if threshold criteria is met.
  	boolean criteria = false;
  	switch (type) {
  	case BOTH:
  	criteria = bothLinksAreShorterThanThreshold(inLink, outLink, thresholdLength);
  	break;
  	case EITHER:
  	criteria = eitherLinkIsShorterThanThreshold(inLink, outLink, thresholdLength);
  	break;
  	default:
  	break;
  	}
  	// yyyy The approach here depends on the sequence in which this goes through the nodes:
  	// * in the "EITHER" situation, a long link may gobble up short neighboring links
  	// until it hits another long link doing the same.
  	// * In the "BOTH" situation, something like going through nodes randomly will often merge
  	// the neighboring links, while going through the nodes along some path will mean that it will
  	// gobble up until the threshold is met.
  	// I would strongly advise against setting thresholdLength to anything other than POSITIVE_INFINITY.
  	// kai, feb'18

• matsim-libs/matsim/src/main/java/org/matsim/core/network/algorithms/NetworkSimplifier.java

  Lines 223 to 234 in 2c5cd05

  	// Only merge if threshold criteria is met.
  	boolean isHavingShortLinks = false;
  	switch (type) {
  	case BOTH:
  	isHavingShortLinks = bothLinksAreShorterThanThreshold(inLink, outLink, thresholdLength);
  	break;
  	case EITHER:
  	isHavingShortLinks = eitherLinkIsShorterThanThreshold(inLink, outLink, thresholdLength);
  	break;
  	default:
  	break;
  	}

The latter two parts have no function if someone uses a thresholdLength = positive infinity as recommended in Kai's comment.

[vsp-gleich]

Hi @marecabo , thanks for taking up my suggestion!

I tend to say yes you can remove the deprecated method, but please keep Kai's comments somehow. The NetworkSimplifierTest also uses the deprecated run method, so removing the deprecated run method will cause some work over there too.