Skip to content
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

Switch matsim version to 2024 #47

Merged
merged 1 commit into from
Apr 18, 2024
Merged

Conversation

Janekdererste
Copy link
Member

  • remove compilation errors
  • Switch to junit 5

Test fails, because events are different

 * remove compilation errors
 * Switch to junit 5

Test fails, because events are different
@Janekdererste
Copy link
Member Author

Janekdererste commented Apr 14, 2024

The test fails because the events file is not as expected. The vehicles seem to have other links assigned than before. Does anybody have an idea what has changed and whether this behavior is expeted or not?

image

@kainagel @mrieser @rakow @paulheinr

@kainagel
Copy link
Member

kainagel commented Apr 15, 2024

Looking at the code, this is the equil scenario, with everybody starting on the central route. The test only does one iteration, during which 10% of agents should be replanned onto a different route. The excerpt look like a different set of 10% was picked.

If this assessment is correct, then picking a different 10% is not a problem content-wise. However, why do we get a different random number sequence? And why did that not happen with tests in the matsim core?

(I think that KMT and I actually changed code around the StrategyManager, since it was internally inconsistent, and as a result difficult to use for Carrier. Maybe we by accident introduced threading race conditions? But this is really only a wild guess. I cannot say if this should have been detected upstream, or even if the test we are presently looking at uses multiple threads.)

@Janekdererste Janekdererste merged commit a574684 into master Apr 18, 2024
1 check passed
@Janekdererste Janekdererste deleted the switch-to-2024-release branch April 18, 2024 12:13
@paulheinr
Copy link
Collaborator

paulheinr commented May 6, 2024

I dug into that problem. The test starts failing with this PR in matsim-libs: matsim-org/matsim-libs#2827, which changes the default routing algorithm. It's a bit strange that this effects the routing results.

However, some test files had to be changed in that PR. @mrieser Do you remember why? And what do you think about the changing routes here?

@paulheinr
Copy link
Collaborator

Update: With AStarLandmarks as implementation, the estimated remaining cost of all links differ a little bit. Since link 3 had the smallest estimate, it was picked. With SpeedyALT, all estimates are the same (which is the expected behavior since this example uses only the freespeed travel time in each iteration and this is the same for the relevant links).

Interestingly, in the test case introduced in matsim-org/matsim-libs#3259, both algorithms find the same route (using links 2 and 11). I don't know why AStarLandmarks behaves differently here, but it's not relevant anymore, since the speedy version is now the standard 🙃

paulheinr added a commit that referenced this pull request May 8, 2024
Isaacwilliam4 pushed a commit to Isaacwilliam4/rl-matsim-example-project that referenced this pull request Oct 24, 2024
Isaacwilliam4 pushed a commit to Isaacwilliam4/rl-matsim-example-project that referenced this pull request Oct 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants