matsim-libs icon indicating copy to clipboard operation
matsim-libs copied to clipboard

MATSim 13.0: Incompatibility of SwissRailRaptor with custom MainModeIdentifier

Open tschlenther opened this issue 3 years ago • 3 comments

MATSim 13.0 might have an incompatiblity issue: If somebody uses transit and configures the RoutingAlgorithmType to be SwissRailRaptor, the SwissRailRaptorModule is automatically bound (since commit e242664b). In Versions before commit c7f1761a though, the SwissRailRaptorModule binds its own MainModeIdentifier, i.e. in the MATSim 13.0 release.

Here is the thing (which i still do not fully understand): The user can not override the binding of the MainModeIdentifier (in the usual way of declaring adding an overriding module to the Controler). If one tries that, an exception is thrown inside the SwissRailRaptor at the line of the MainModeIdentifier binding, saysing that this interface has already bound at another place. To me, it seems like the binding of the overriding module was processed before the binding in the SwissRailRaptorModule. This might be, because the SwissRailRaptorModule is now part of the ControlerDefaultsModule pipeline. But i might be totally wrong: Guice does its own magic.

I have tested various setups and runs (within matsim-scnearios/matsim-hamburg) and came to the point that for MATSim versions between the above mentioned commits, one can probably not use the SwissRailRaptorModule with another MainModeIdentifier. Now, drt uses its own MainModeIdentifier, too. As do probably more modules and scenarios. If that suspicion is correct, that means that all of the (nowadays many) scenarios relying on the SwissRailRaptor can not be run with drt, when using MATSim 13.0. I have checked the RunDRTExampleTestIT, and it understandably does not use transit. I am not sure whether there is a test combining the SwissRailRaptor with a custom MainModeIdentifier inside of MATSim. It might be, that this is why this issue has found its way into the release.

tschlenther avatar Jun 22 '21 15:06 tschlenther

Just some notes:

The custom MainModeIdentifier was only bound by SwissRailRaptor if intermodalAccessEgress was enabled. This should at least limit the number of affected people a bit.

As for why it's not possible to overwrite it, I don't know. I would have expected that it is possible, but I'm no expert on that DI stuff.

With the introduction of the routingMode attribute, the MainModeIdentifier should be used less and less, I think only for backwards compatibility (e.g. reading old plans files without the routingMode attribute) or for analysis. So I would expect that in many cases a custom MainModeIdentifier is no longer necessary for new code (unless the DRT contrib uses the MainModeIdentifier internally somehow which I'm not aware of).

It might make sense to back port commit c7f1761 to the 13.x branch, so this fix will be included in an upcoming 13.1 release.

mrieser avatar Jun 22 '21 18:06 mrieser

The custom MainModeIdentifier was only bound by SwissRailRaptor if intermodalAccessEgress was enabled. This should at least limit the number of affected people a bit.

Absolutely right. Thanks for pointing that out.

As for why it's not possible to overwrite it, I don't know. I would have expected that it is possible, but I'm no expert on that DI stuff.

Yes, i still can't wrap my head around that. I wonder if somebody else can?

With the introduction of the routingMode attribute, the MainModeIdentifier should be used less and less, I think only for backwards compatibility (e.g. reading old plans files without the routingMode attribute) or for analysis. So I would expect that in many cases a custom MainModeIdentifier is no longer necessary for new code (unless the DRT contrib uses the MainModeIdentifier internally somehow which I'm not aware of).

Again, absolutely right. So that limits the number of affected people again. But one has to know, that a MainModeIdentifier is no longer necessary..

It might make sense to back port commit c7f1761 to the 13.x branch, so this fix will be included in an upcoming 13.1 release.

I cherry-picked that commit in #1564. Will a 13.1 release definetely come?

tschlenther avatar Jun 23 '21 08:06 tschlenther

i merged #1564 such that c7f1761 is in the 13.x branch. Will leave this issue open though, until 13.1 is released.

tschlenther avatar Jun 25 '21 07:06 tschlenther