Round environment #48

vagechirkov · 2022-11-07T13:58:58Z

vagechirkov · 2022-11-07T14:07:41Z

I'm wondering how to make better videos 😅

vagechirkov · 2022-11-07T14:56:55Z

I think the problem with the workflow is because of this:

I'm not sure how we can fix this... 🤔

@mezdahun

mezdahun · 2022-11-08T13:22:54Z

@vagechirkov that arena looks awesome! 🤩

regarding the Docker CLI automation you could try to add the missing credentials to your github account (in case you have DockerHub credentials). If that doesn't work, from now on you could make indirect PRs to first just feature branches in the main repo instead of direct PR from fork feature branch to develop in original repo. This automation is only executed on pushes and PRs to origin/develop and is testing installable dependencies on docker environment. Currently if you change e.g. setup.py or Dockerfile in a way that it is messing up the docker image there is no way to catch it before merging to develop as the test must be manually bypassed.

vagechirkov · 2022-11-08T14:31:27Z

@vagechirkov that arena looks awesome! 🤩

regarding the Docker CLI automation you could try to add the missing credentials to your github account (in case you have DockerHub credentials). If that doesn't work, from now on you could make indirect PRs to first just feature branches in the main repo instead of direct PR from fork feature branch to develop in original repo. This automation is only executed on pushes and PRs to origin/develop and is testing installable dependencies on docker environment. Currently if you change e.g. setup.py or Dockerfile in a way that it is messing up the docker image there is no way to catch it before merging to develop as the test must be manually bypassed.

Hmm, I see. I have the credentials in my fork and the workflow works fine. For example, I enabled workflow for round-environment branch and Docker Image CI works fine https://github.com/vagechirkov/ABM/tree/round-environment but not in the PR from the fork (probably because of the security issues).

vagechirkov · 2022-11-08T14:56:04Z

We can run Docker CI on all branches or all branches with the specific pattern (e.g. "develop/**") but not on the pull request. I don't think it's possible to get secrets from the fork to run a workflow for PR. @mezdahun what do you think? In principle, we can first push into another branch and then merge into the develop branch, but I'm not sure if that's an optimal solution 🤔.

mezdahun · 2022-11-08T14:57:33Z

I see! In that case the preferred workflow would be to not PR directly to develop in the original repo but rather to a feature branch first. In that PR general tests are run. Then I PR in the original repo the feature branch to develop and check Docker CLI. Would that work for you?

mezdahun · 2022-11-08T14:59:22Z

@vagechirkov What do you mean by it's not the optimal solution?

mezdahun · 2022-11-08T15:05:34Z

@vagechirkov danger/danger-js#918 (comment) I guess something like this would be nicer, but it wouldn't solve the problem I mentioned before, namely we merge without checking if the resulting docker image is healthy

mezdahun · 2022-11-08T15:13:47Z

Another option is to do direct PRs from your fork to develop but I will check the result of Docker CLI test on your feature branch before bypassing the check on the PR.

vagechirkov · 2022-11-08T15:22:03Z

@vagechirkov What do you mean by it's not the optimal solution?

Well, in principle I think it's ok to go through the feature branch before merging commits with the main branch. It's just a bit redundant. 🙃

vagechirkov · 2022-11-08T15:37:36Z

I'm wondering why this is necessary to run Docker CI before merging changes to the repo. I think we can run tests of the code with pytest in the different workflow and use the Docker CI workflow just to push a new image to Docker Hub. 🤔

mezdahun · 2022-11-08T16:13:47Z

The idea behind is I guess that we want to keep our image on DockerHub always in sync with develop (as this will serve as the environment for HPC simulations). In case something is merged that messes up the image they get out of sync and you have to fix that on a different branch again (after merge). This is why already on the PR side it is checked. What we could do here due to the credential issues from fork is to instead separate the docker build and the push in 2 different github action files. This way the docker build would run on the PR side allowing us to be sure that the resulting image would be okay. The one with push to DockerHub would then be only activated upon/after merge. This would be also relatively easy I think. Which one would be your preferred option?

PR to feature branches instead of develop
Check docker result on remote branch then bypass check at PR to develop
Separate Docker check to 2 checks (1 on PR only build, 2 on merge build and push)

mezdahun · 2022-11-08T16:14:14Z

Or you meant the same thing?

vagechirkov · 2022-11-08T17:40:33Z

Or you meant the same thing?

Yes indeed, I like the third option! Let me try to build the image without login :)

vagechirkov · 2022-11-08T17:47:05Z

Cool! I think this should work! It makes a lot of sense to only push the Docker image to Docker Hub after merging PR.

mezdahun · 2022-11-09T07:28:37Z

Awesome, I will do a review later and we can merge this!

mezdahun

Awesome! A few things which we should fix before merging is:

fix placing new agents into circular arena when playground tool is used
update the position of the agents only once per time step
maybe comment a bit on why that check of new angle is needed. i.e. how it can happen that the calculation yields a new angle that points outside of the circle?
remove magic numbers
BUG: if the resource patch is small and close to the border while user increases the patch radius (detection range) the border gets stuck at the wall.
Discuss: seemingly the resulting angles are very sharp. We could think about a recursive turning method inside? Maybe also not necessary if this approach works.

mezdahun · 2022-11-10T09:30:35Z

abm/projects/cooperative_signaling/cs_agent/cs_agent.py

+        self.prove_orientation()
+
+        # relocate the agent back inside the circle
+        self.position[0] += (self.radius / 2) * np.cos(self.orientation)


We are changing the position of the agents twice in a single time step. Once in self.update() and once here.

What do you think about this solution? b990a13

mezdahun · 2022-11-10T09:31:37Z

abm/projects/cooperative_signaling/cs_agent/cs_supcalc.py

+
+    # make sure that the new orientation points inside the circle and not too
+    # flat to the border
+    if np.abs(new_orientation - i_orientation) > np.pi / 4:


I understand this fixes the behavior, but I am wondering why it is necessary.

I would guess it has something to do with the curvature of the boundary and the step size per update 😅

mezdahun · 2022-11-10T09:33:48Z

abm/projects/cooperative_signaling/cs_agent/tests/test_cs_supcalc.py

@@ -12,3 +12,24 @@ def test_random_walk():

    assert dvel == 1.0


for me random walk test fails can you please check?

Ah it is because the env parameters, we might want to mock this so that a new local env file won't influence the test results

This is strange because all the tests in the workflow passed 🤔

If you replace the .env file (i.e. you are after running the playground tool) the tests won't pass. Or do they pass for you even with a new .env file?

I never had any problems with tests before or after running the playground 😅🤔

abm/projects/cooperative_signaling/cs_environment/cs_resource.py

abm/projects/cooperative_signaling/cs_simulation/cs_sims.py

vagechirkov · 2022-11-11T10:14:22Z

@mezdahun Please let me know if there is anything else we need to fix before merging this PR. I'm not sure why tests are failing for you, but I think we're good as tests are passing in the GitHub workflow 🙂

mezdahun · 2022-11-11T13:04:24Z

In the github workflow tests pass because it uses the same prototype .env file as always. Once you change your env file locally and run pytest they will fail. (as soon as you change env parameters that your tests use via the function to be tested)

Usually this should be mocked, as in env parameters are fixed for the given run of a test case. I guess this si our of scope here, it is enough if we make the tests for the prototype env file.

Should we look into this bug with increasing the detection range of the resource around the wall?

To reproduce: Stop the simulation (with space) and move the resource patch near the wall. Increase the detection range so that the patch is now out of the arena partially. Resume the simulation (with space).

Expected behavior: the resource is getting relocated and bouncing back from wall of arena

Behavior: The patch gets stuck and won't move anymore

vagechirkov · 2022-11-11T13:48:59Z

In the github workflow tests pass because it uses the same prototype .env file as always. Once you change your env file locally and run pytest they will fail. (as soon as you change env parameters that your tests use via the function to be tested)

Usually this should be mocked, as in env parameters are fixed for the given run of a test case. I guess this si our of scope here, it is enough if we make the tests for the prototype env file.

Should we look into this bug with increasing the detection range of the resource around the wall?

To reproduce: Stop the simulation (with space) and move the resource patch near the wall. Increase the detection range so that the patch is now out of the arena partially. Resume the simulation (with space).

Expected behavior: the resource is getting relocated and bouncing back from wall of arena

Behavior: The patch gets stuck and won't move anymore

Could you fix this .env file problem? Unfortunately, I cannot reproduce it in my setup. I will fix the relocation of recourse inside the arena 🙂

This reverts commit 377b298.

…ple)

tests consistent with even changed .env files

mezdahun · 2022-11-14T09:47:29Z

In the github workflow tests pass because it uses the same prototype .env file as always. Once you change your env file locally and run pytest they will fail. (as soon as you change env parameters that your tests use via the function to be tested)
Usually this should be mocked, as in env parameters are fixed for the given run of a test case. I guess this si our of scope here, it is enough if we make the tests for the prototype env file.
Should we look into this bug with increasing the detection range of the resource around the wall?
To reproduce: Stop the simulation (with space) and move the resource patch near the wall. Increase the detection range so that the patch is now out of the arena partially. Resume the simulation (with space).
Expected behavior: the resource is getting relocated and bouncing back from wall of arena
Behavior: The patch gets stuck and won't move anymore

Could you fix this .env file problem? Unfortunately, I cannot reproduce it in my setup. I will fix the relocation of recourse inside the arena slightly_smiling_face

09f67da should fix this. I mocked the movementparams submodule's fixed variables that is used by the random walk function if no parameters are passed to it. In that case (when function parameters are None) it fetched the movementparams submodules corresponding attributes (velocity, min and max theta). These are read from the root .env file upon startup of the stack.

mezdahun

🚀

vagechirkov · 2022-11-14T09:55:19Z

LGTM, let's merge! However, I don't have a merge button 😂

mezdahun · 2022-11-14T09:56:46Z

Oh sorry I thought you had one

vagechirkov · 2022-11-14T10:05:35Z

Oh sorry I thought you had one

This is probably set in the protected branch settings 🤔

vagechirkov added 5 commits November 4, 2022 16:54

update draw walls function

dfb9dc1

add reflection_from_circular_wall function and tests

ac9e493

reflect agent from the circular border

d08a7e3

reflect resource from the circular border

3a073dd

minor comment

e61ecca

ignore videos

9b3e07f

run test on PR too

c40ff66

test docker-image workflow

4fde313

split GitHub action Docker CI workflow in two workflows

465e8bb

mezdahun requested changes Nov 10, 2022

View reviewed changes

vagechirkov added 4 commits November 10, 2022 15:16

Merge branch 'develop' into round-environment

5f486f6

update agent's position only ones per update

b990a13

relocate resource base on its velocity

768941d

fix new agents generation via slider in playground

12a35dc

vagechirkov requested a review from mezdahun November 11, 2022 10:14

vagechirkov added 2 commits November 14, 2022 09:16

make the default patch_radius larger

377b298

Revert "make the default patch_radius larger"

b74d604

This reverts commit 377b298.

vagechirkov mentioned this pull request Nov 14, 2022

5a Clean up Simulation parameters #50

Open

vagechirkov and others added 6 commits November 14, 2022 09:46

refactor imports

fdd57fd

fix the bug when moving agents with the mouse (use list instead of tu…

d3359aa

…ple)

use velocity instead of radius for relocation of the agent

8e02862

relocate agents when their positions are manually changed

887199b

relocate resource when the positions are manually changed

edf6d39

Mocking submodule variables to keep

09f67da

tests consistent with even changed .env files

mezdahun approved these changes Nov 14, 2022

View reviewed changes

mezdahun merged commit 0c687c2 into scioip34:develop Nov 14, 2022

vagechirkov deleted the round-environment branch November 14, 2022 10:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Round environment #48

Round environment #48

vagechirkov commented Nov 7, 2022

vagechirkov commented Nov 7, 2022

vagechirkov commented Nov 7, 2022

mezdahun commented Nov 8, 2022

vagechirkov commented Nov 8, 2022 •

edited

Loading

vagechirkov commented Nov 8, 2022

mezdahun commented Nov 8, 2022

mezdahun commented Nov 8, 2022 •

edited

Loading

mezdahun commented Nov 8, 2022 •

edited

Loading

mezdahun commented Nov 8, 2022

vagechirkov commented Nov 8, 2022

vagechirkov commented Nov 8, 2022

mezdahun commented Nov 8, 2022

mezdahun commented Nov 8, 2022

vagechirkov commented Nov 8, 2022

vagechirkov commented Nov 8, 2022

mezdahun commented Nov 9, 2022

mezdahun left a comment

mezdahun Nov 10, 2022

vagechirkov Nov 10, 2022

mezdahun Nov 10, 2022

vagechirkov Nov 10, 2022

mezdahun Nov 10, 2022

mezdahun Nov 10, 2022

vagechirkov Nov 10, 2022

mezdahun Nov 11, 2022

vagechirkov Nov 11, 2022

vagechirkov commented Nov 11, 2022

mezdahun commented Nov 11, 2022

vagechirkov commented Nov 11, 2022

mezdahun commented Nov 14, 2022

mezdahun left a comment

vagechirkov commented Nov 14, 2022

mezdahun commented Nov 14, 2022

vagechirkov commented Nov 14, 2022

		@@ -12,3 +12,24 @@ def test_random_walk():

		assert dvel == 1.0

Round environment #48

Round environment #48

Conversation

vagechirkov commented Nov 7, 2022

vagechirkov commented Nov 7, 2022

vagechirkov commented Nov 7, 2022

mezdahun commented Nov 8, 2022

vagechirkov commented Nov 8, 2022 • edited Loading

vagechirkov commented Nov 8, 2022

mezdahun commented Nov 8, 2022

mezdahun commented Nov 8, 2022 • edited Loading

mezdahun commented Nov 8, 2022 • edited Loading

mezdahun commented Nov 8, 2022

vagechirkov commented Nov 8, 2022

vagechirkov commented Nov 8, 2022

mezdahun commented Nov 8, 2022

mezdahun commented Nov 8, 2022

vagechirkov commented Nov 8, 2022

vagechirkov commented Nov 8, 2022

mezdahun commented Nov 9, 2022

mezdahun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vagechirkov commented Nov 11, 2022

mezdahun commented Nov 11, 2022

vagechirkov commented Nov 11, 2022

mezdahun commented Nov 14, 2022

mezdahun left a comment

Choose a reason for hiding this comment

vagechirkov commented Nov 14, 2022

mezdahun commented Nov 14, 2022

vagechirkov commented Nov 14, 2022

vagechirkov commented Nov 8, 2022 •

edited

Loading

mezdahun commented Nov 8, 2022 •

edited

Loading

mezdahun commented Nov 8, 2022 •

edited

Loading