Closed rikifunt closed 3 months ago
Thank you for the comments and clarifications! I will address them and continue working on this as soon as I can.
It makes a lot of sense to keep the dynamics agnostic of the action space. To keep compatibility with the current action spaces, it should be possible to rewrite the multidiscrete -> continuous conversion in Environment._set_action
so that the discretization logic stays the same for n=3, I'll look into it.
At this point, the overall changes would be:
action_nvec
parameter to Agent.__init__
(optional and mutually exclusive with action_size
)Environment
and sampling random actions according to action_nvec
action_nvec
in Environment._set_action
, keeping the current logic when n=3I will also look into interactive_render (I initially left it untouched because it uses continuous actions). Let me know if there is other functionality that you think these changes might break.
Thank you for the comments and clarifications! I will address them and continue working on this as soon as I can.
It makes a lot of sense to keep the dynamics agnostic of the action space. To keep compatibility with the current action spaces, it should be possible to rewrite the multidiscrete -> continuous conversion in
Environment._set_action
so that the discretization logic stays the same for n=3, I'll look into it.At this point, the overall changes would be:
- adding the
action_nvec
parameter toAgent.__init__
(optional and mutually exclusive withaction_size
)
Yes, you basically have already done this. I would not make them mutually exclusive. They will default to None
. If one is specified, we create the other one based on that one. If both are specified, we check they are coherent. If none are specified we default to dynamics (and the old 3 way split)
Also maybe it is worth naming this with something that contains "discrete" like discrete_action_nvec
to highlight that is not something that interests users that have continuous actions (which I think is majority)
- building discrete spaces in
Environment
and sampling random actions according toaction_nvec
yes, merge main and this should come almost for free with what you have already done
- using
action_nvec
inEnvironment._set_action
, keeping the current logic when n=3
so here is where i am still a bit undecided.
what I suggest is that we split the range among the n actions.
When n
is odd, we make the first action being 0 for bc-compatibility.
This would make so that n=3 is not a special case but the way we handle odd n
s
For example:
n=2
leads to [-range,range]
n=3
leads to [0,-range,range]
n=4
leads to [-range,-range/3,range/3,range]
n=5
leads to [0,-range,-range/2,range/2,range]
- adding tests where needed
Yeah thanks. the main tests I would like to see is regarding the logic that will be added to environemnt._set_action
in different settings
Then, as you said, we can delegate any extra action semantics to process_action. Are these ok?
yep!
I will also look into interactive_render (I initially left it untouched because it uses continuous actions). Let me know if there is other functionality that you think these changes might break.
Oh yes sorry I forgot interactive_render uses continuous, then we should be good there
I added 2 commits with the suggested changes (and merged main):
action_nvec
is now only present in Agent
and is called discrete_action_nvec
(changes to dynamics are reverted)discrete_action_nvec
is not mutually exclusive with action_size
, but we check that they are consistent in Agent.__init__
_set_action
handles odd n
s by converting the first discrete action to u = 0
, and shifting the remaining "negative" actions by -1 so that the previous logic applies_set_action
is now accurate (turns out the logic was already the same as before the generalization)Overall, this results in the following "multidiscrete a
" to "continuous u
" conversions (where U = u_max
):
n
is odd (and let h = (n-1)/2
, i.e. the middle point of the discrete possibilities):
a == 0 ==> u == 0
a in [1, h] ==> u in [-U, -U/h]
a in [h+1, n) ==> u in [+U/h, U]
n
is even
a in [0, n] ==> u in [-U, U]
I am now working on adding tests that check the above logic and the discrete-multidiscrete conversion, and I was wondering if it is possible to build a Scenario
object directly instead of passing a string to make_env
? This would make it possible to set the nvec
s to values other than 3 before constructing the Environment
object, so that various values can be tested. Currently I am trying to do something like this in tests/test_vmas.py
(where the load(scenario)
line is taken from make_env
):
from vmas import scenarios
...
@pytest.mark.parametrize("scenario", scenario_names())
def test_discrete_action_nvec_multidiscrete(scenario, num_envs=10, n_steps=10):
scenario = scenarios.load(scenario).Scenario()
random.seed(0)
for agent in scenario.agents:
agent.discrete_action_nvec = [random.randint(2, 6) for _ in range(agent.action_size)]
env = make_env(
scenario=scenario,
num_envs=num_envs,
seed=0,
multidiscrete_actions=True,
continuous_actions=False,
)
for _ in range(n_steps):
...
However, I get an error on the first line of the function, saying that scenarios
is a list
and does not have the load
method. I suspect this has something to do with the python import machinery? I also tried using vmas.scenarios
directly instead of importing it at the top, with the same result. From what I can see, vmas.scenarios
is both a subpackage in the vmas
directory and a list defined in vmas/__init__.py
, so maybe this is the problem? Is there any other way to accomplish this?
Fantastic work!
It seems to be exactly what we want
I am now working on adding tests that check the above logic and the discrete-multidiscrete conversion, and I was wondering if it is possible to build a
Scenario
object directly instead of passing a string tomake_env
? This would make it possible to set thenvec
s to values other than 3 before constructing theEnvironment
object, so that various values can be tested. Currently I am trying to do something like this intests/test_vmas.py
(where theload(scenario)
line is taken frommake_env
):from vmas import scenarios ... @pytest.mark.parametrize("scenario", scenario_names()) def test_discrete_action_nvec_multidiscrete(scenario, num_envs=10, n_steps=10): scenario = scenarios.load(scenario).Scenario() random.seed(0) for agent in scenario.agents: agent.discrete_action_nvec = [random.randint(2, 6) for _ in range(agent.action_size)] env = make_env( scenario=scenario, num_envs=num_envs, seed=0, multidiscrete_actions=True, continuous_actions=False, ) for _ in range(n_steps): ...
However, I get an error on the first line of the function, saying that
scenarios
is alist
and does not have theload
method. I suspect this has something to do with the python import machinery? I also tried usingvmas.scenarios
directly instead of importing it at the top, with the same result. From what I can see,vmas.scenarios
is both a subpackage in thevmas
directory and a list defined invmas/__init__.py
, so maybe this is the problem? Is there any other way to accomplish this?
if you do from vmas import scenarios
it will load the scenario list.
What you want is the module:
@pytest.mark.parametrize("scenario", scenario_names())
def test_prova(scenario, num_envs=10, n_steps=10):
from vmas.scenarios import load
scenario_class = load(f"{scenario}.py").Scenario()
env = make_env(
scenario=scenario_class,
num_envs=num_envs,
seed=0,
multidiscrete_actions=True,
continuous_actions=False,
)
for _ in range(n_steps):
env.step(env.get_random_actions())
this will work
n=1
is not currently handled properly and results in NaNs
should this even be allowed? I would introduce a check that reads the nvec and checks all values are >= 2
Thanks for the comments! I've committed the suggested changes and fixed the bug in the discrete -> multi-discrete conversion
n=1
is not currently handled properly and results in NaNsshould this even be allowed? I would introduce a check that reads the nvec and checks all values are >= 2
Yes, this should not happen, I added an input sanity check in Agent.__init__
.
To make sure all of this works as intended, I've added some tests that check:
Environment._set_action
Environment._set_action
These are run for all scenarios that don't explicitly override process_action
(since it breaks the action <-> control mapping). nvec
s are randomly sampled (with a fixed seed) in the interval [2,6] to properly test mixed odd-even nvec
s and odd n != 3. Let me know if there is anything else that needs testing.
This PR supports specifying different number of actions for each action dimension when the action space is discrete. The motivation is to support action dimensions with different semantics that require different discretizations for each dimension (e.g. combining navigation and "boolean" actions). More in general, this should allow to add any kind of discrete action with arbitrary semantics (also thanks to process_action).
To implement this, I added an
action_nvec
property toDynamics
, which is "mutually exclusive" withneeded_action_size
, meaning that if one is overridden the other is computed automatically, and vice-versa (they can't be both overridden). By default,action_nvec
will be a list of 3s of lengthaction_size
when not specified, to keep compatibility with existing scenarios. I also addedaction_nvec
to theAgent
class, which is then used inEnvironment
to build the discrete action space(s) and inEnvironment._set_action
to: 1) convert discrete actions in the interval [0, n) to [-u_max, u_max]; and 2) convert discrete actions to multi-discrete ones when needed.I also added a
Composite
subclass ofDynamics
that allows to compose multipleDynamics
into one by concatenating their actions, which should make it more convenient (modular) to define complex actions/dynamics.Let me know if there is anything else that can improve the PR.