Skip to content

Conversation

@sosiristseng
Copy link
Contributor

@sosiristseng sosiristseng commented Aug 14, 2022

fix #675

I also renewed the facing logic to:

!(dot(r2, v1) > 0 && dot(r1, v2) > 0) && return false

@sosiristseng
Copy link
Contributor Author

However, some disks would pass each other. Is there any tip so I can fix that?

socialdist2.mp4

@sosiristseng
Copy link
Contributor Author

The current facing logic does not consider the lower one.
Elastischer_stoß2

@sosiristseng
Copy link
Contributor Author

The new logic using dv looks better but fails to pass one collision test.

socialdist2.mp4

@sosiristseng
Copy link
Contributor Author

I'm reading these lines in tests.

# x, which is the changed velocities
# should be at least the amount of collisions happened divided by 2
# because half the agents are unmovable (in a collision at most one agent
# must change velocity)
@test x > 0
@test model.c > 0
@test x model.c/2

In the tests, only 50 out of 100 agents are movable, so x is at most 50. However, they can collide many times in the simulation (167 times in my case). Should we change the test case @test x ≥ model.c/2?

@Datseris
Copy link
Member

Thanks so much, this is a great fix!

Yes please, go ahead and correct the test.

@Datseris
Copy link
Member

Ummm @sosiristseng are you perhaps using an automated linter? Can you please revert the linter induced changes? We do not use a linter yet in Agents.jl, but will in the future. Until then, it is best to not include linting changes in pull requests.

@sosiristseng
Copy link
Contributor Author

sosiristseng commented Aug 15, 2022

I left my formatter on. Will fix that, too.

Edit: It's done. Please have a look.

@codecov-commenter
Copy link

codecov-commenter commented Aug 15, 2022

Codecov Report

Merging #676 (46b9155) into main (8312904) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #676   +/-   ##
=======================================
  Coverage   90.42%   90.42%           
=======================================
  Files          27       27           
  Lines        1733     1733           
=======================================
  Hits         1567     1567           
  Misses        166      166           
Impacted Files Coverage Δ
src/spaces/continuous.jl 92.89% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@Datseris Datseris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much, we appreciate the fix!

@Datseris Datseris merged commit 4380ba9 into JuliaDynamics:main Aug 15, 2022
@sosiristseng
Copy link
Contributor Author

Hello, since this is a correctness fix, would you deploy a patch version (like 5.4.4) containing this PR? Or would you like this PR to go into a bigger new version (like 5.5.0) with other PRs?

@Datseris
Copy link
Member

Datseris commented Sep 4, 2022

I will release 5.5 soon with this in. Can't promise when though! At the moment the main branch is already too far away from 5.4 to publish a patch release. The mean time, feel free to use the development version!

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.

The social distancing model does not look like elastic collision

3 participants