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

[WIP] Modify collision parameters #35

Closed
wants to merge 2 commits into from
Closed

Conversation

corentinlger
Copy link
Collaborator

@corentinlger corentinlger commented Mar 8, 2024

Description

Small modifications in collision parameters (the ones of the soft_sphere energy function) to make the simulation look less buggy with particle bumps. Artificially increased the sigma parameter (diameter of the particles), increased the epsilon (interaction energy scale) and decreased the alpha (interaction stiffness) to prevent particles from getting too close (it seemed like they were sometimes only colliding when the center of the particles were really close before).

It made me notice that there are some 'weird' interactions with the squared objects (that are in fact round in the simulation computation), maybe we could add a special a special epsilon or alpha parameter for the objects to prevent agents to run into them and only be rejected when they are really close.

It also made me think we could add a module like argparse to enable specifying the simulation parameters when launching the simulator in cmd line.

Related Issue (if applicable)

How to Test

Launch the server

I changed this part in the last commit :

python3 simulator_server.py

Launch the Panel interface

panel serve vivarium/interface/panel_app.py --autoreload

Check if the simulation seems smoother with the new collisions between particles. Does it seem better @Marsolo1 @clement-moulin-frier ?

Screenshots (if applicable)

@corentinlger
Copy link
Collaborator Author

I refactored the code to only launch the simulation (in local or server mode) from python files in the root directory (now run simulator_server.py instead of vivarium/simulator/grpc_server/simulator_server.py and simulate.py instead of vivarium/simulator/simulator.py). I added an argparse option to choose the collision parameters from cmd-line (there are also default values), it will surely be useful for other parameters in the future.

From there I added another parameter col_params to the Simulator class to specify the collision parameters in the simulation dynamics function. I also changed a few functions in sim_computation.py to take as argument this new parameter instead of the old default values encoded directly in the collision functions.

It made me realize there was a bug when setting the collision parameters alphadirectly in the sim_computation.py file, I set it to f32(alpha) to fix the error. I am not sure but I think it wasn't used and was maybe set to its default value (1.) before. I therefore tuned the collision parameters a bit to make them more 'realistic'.

Now there are a few conflicts (because I removed the code to launch the simulations from files in the vivarium/simulator/ directory), I can resolve them and merge if you are ok with the other changes.

@clement-moulin-frier
Copy link
Collaborator

Thanks @corentinlger

There are a few things to be discussed with this PR I think:

  1. Regarding tuning the collision parameters: I'm a not a fan of the solution with dist_mul that artificially changes the particule diameters, we should better understand why it doesn't work as expected with correct diameters.
  2. Regarding how to set the collision parameters, why not being able to set them through argparse indeed. But before that, they should be accessible in the configs and the state as the other simulation parameters. This will also simplify the code in sim_computationas you won't have to pass them as function arguments (they will be directly accessible from state).
  3. Regarding the new files simulate and simulator_server: what is the role of these files? Two possibilities I think:
    • They are used to launch the simulator or the server from command line. In that case it is indeed a good idea to make them available at root level. This will be useful to launch the server (better to call the file run_server.pythough), but I am not sure if it is relevant to just launch the simulator. For this second case, I would instead propose to add imports in the root __init__.py, such that we can do from vivarium import SimulatorServer instead of from vivarium.simulator.simulator import Simulator, which will make easier for users to write their own script.
    • Or, they are test files, but in that case they should reside in the testsdirectory.

So in summary, I suggest to focus this PR on point 1 and the non-argparse part of point 2 above. He we first need to better understand how collision works, as well as to store collision parameters in the state and configs (or alternatively as constants in sim_computationto start with a simpler solution, as it is already the case with SPACE_NDIMS). Actually, having them accessible in configs (and therefore in the interface) will allow to experience on the effect of those parameters in real time and might help to tune them correctly. We should discuss if they are more global or entity-specific parameters though (the difficulty here is that they are actually related to pairs of particules, so it deserves a discussion I think)

Regarding point 3 and the use of argparse in point 2, I suggest to create a new issue, potentially linked to a first draft PR with your current code about it (or we can discuss it first). You can also copy-paste my related comments there.

In general: try to limit the scope of your PRs to the scope of their associated issue, it will facilitate their review a lot ;)

@corentinlger corentinlger changed the title Modify collision parameters [WIP] Modify collision parameters Mar 13, 2024
@corentinlger corentinlger deleted the corentin/collisions branch March 18, 2024 11:13
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.

2 participants