Adding default agentmode for ithor objectnav task #307

kshitijd20 · 2021-08-24T15:36:40Z

Added config to train objectnav baseline in iTHOR using default agentmode. The following files were updated

allenact/projects/tutorials/object_nav_ithor_ppo_baseline.py contains the experiment config
allenact/allenact_plugins/ithor_plugin/ithor_environment.py added environment arguments e.g. rotatestepdegrees and snaptogrid to pass to the controller
allenact/allenact_plugins/ithor_plugin/ithor_task_samplers.py added objectnavdataset sampler

lgtm-com · 2021-08-24T16:08:36Z

This pull request introduces 6 alerts when merging 24fb1d1 into 0978361 - view on LGTM.com

new alerts:

5 for Unused import
1 for Unused local variable

jordis-ai2

Other than the default values of the new parameters in ithor_environment (I suggest keeping the respective default ones in THOR), everything else looks good to me 👍

jordis-ai2 · 2021-08-24T16:56:17Z

allenact_plugins/ithor_plugin/ithor_environment.py

@@ -38,11 +38,16 @@ def __init__(
        fov: float = FOV,
        player_screen_width: int = 300,
        player_screen_height: int = 300,
+        gridSize: float = 0.15,


I'm not in favor of changing the default values... I would rather suggest that any experiments using these non-default values explicitly set the corresponding arguments. E.g. I'd keep 0.25 for gridSize, 90 for rotateStepDegrees, and 1.25? (not sure) for visibilityDistance.

Thanks, I updated the default values as you suggested. Where can I find the default value for visibilityDistance?

I think it's actually 1.5:

https://ai2thor.allenai.org/ithor/documentation/initialization/#initialization-visibilitydistance

Other than the arguments that had already been set by Luca, I'd try to keep everything else matching the defaults in the link to keep all other experiment configs working as expected.

I have reset all the default values in the environment init from the link. For the visibility_distance there was a constant defined in this file https://github.com/allenai/allenact/blob/main/allenact_plugins/ithor_plugin/ithor_constants.py which was equal to 1.25 so I am using that Constant instead otherwise all the values are set to default from the link.

Great, thanks!

lgtm-com · 2021-08-24T18:19:40Z

This pull request introduces 6 alerts when merging 37795be into 0978361 - view on LGTM.com

new alerts:

5 for Unused import
1 for Unused local variable

lgtm-com · 2021-08-24T21:20:47Z

This pull request introduces 6 alerts when merging 90ef4f5 into 0978361 - view on LGTM.com

new alerts:

5 for Unused import
1 for Unused local variable

lgtm-com · 2021-08-24T22:23:26Z

This pull request introduces 6 alerts when merging a7c7f0d into 0978361 - view on LGTM.com

new alerts:

5 for Unused import
1 for Unused local variable

Lucaweihs

Some minor suggestions but this looks good to me!

allenact_plugins/ithor_plugin/ithor_environment.py

Lucaweihs · 2021-08-24T23:51:08Z

allenact_plugins/ithor_plugin/ithor_environment.py

+    def reset_object_filter(self):
+        self.controller.step("ResetObjectFilter", renderImage=False)
+
+    def teleport(


Why not use the existing teleport_agent_to function?

I tried it at first but it was giving me a lot of warnings of this type
"Teleportation FAILED but agent still moved (position_dist {}, rot diff {})" So, I was not sure and copied the teleport function from Robothor environment.

Gotcha, it would be nice if you could track down what the issue is, otherwise I would need to be convinced that it's worth having two functions that do roughly the same thing.

Lucaweihs · 2021-08-24T23:54:05Z

allenact_plugins/ithor_plugin/ithor_task_samplers.py

@@ -198,3 +200,218 @@ def set_seed(self, seed: int):
        self.seed = seed
        if seed is not None:
            set_seed(seed)
+
+
+class ObjectNavDatasetTaskSampler(TaskSampler):


Can we call this ObjectNaviThorDatasetTaskSampler? I'd also be up for renaming the existing ObjectNavTaskSampler class in this file to ObjectNaviThorTaskSampler

It seems like a large amount of functionality is shared between this task sampler and the robothor variant. Can we maybe create an abstract superclass for both of these task samplers (containing the shared functions) and have them both subclass it?

I like the idea. We could have that under an allenact/embodiedai/tasks directory.

I have renamed it to ObjectNaviThorDatasetTaskSampler . I was not sure about changing the ObjectNavTaskSampler as it might be used somewhere else and therefore we will have to update it everywhere it was used. So for now, I have avoided that.

For the abstract superclass suggestion, should I create a file allenact/embodiedai/tasks/objectnav.py and create the abstract superclass here?

@kshitijd20 - so that you can merge this and work on other things, let's just keep it as is for now. It would be great if you could add a comment like:

# TODO: Merge functionality of `ObjectNavDatasetTaskSampler` for iTHOR and RoboTHOR.

somewhere there.

@jordis-ai2 - I hadn't thought of going as far as moving it to allenact/embodiedai/tasks but that's an interesting idea. If we want to put it there it might be worth thinking of how we can make this even more general, perhaps we can just have a DatasetTaskSampler? I was also thinking that we should likely merge the robothor and ithor plugins (as they replicate a lot of functionality) into a more general thor plugin.

That sounds really great. As soon as I'm done with my current tasks, I think I will give this priority. If things are done correctly, we might remove quite a bit of lines of code 👍

projects/tutorials/object_nav_ithor_ppo_baseline.py

lgtm-com · 2021-08-25T15:28:34Z

This pull request introduces 6 alerts when merging fe3ed2e into 0978361 - view on LGTM.com

new alerts:

5 for Unused import
1 for Unused local variable

lgtm-com · 2021-08-25T17:43:59Z

This pull request introduces 7 alerts when merging be37e2a into 0978361 - view on LGTM.com

new alerts:

6 for Unused import
1 for Unused local variable

lgtm-com · 2021-08-25T22:05:10Z

This pull request introduces 7 alerts when merging 11bc7cd into 0978361 - view on LGTM.com

new alerts:

6 for Unused import
1 for Unused local variable

Kshitij Dwivedi and others added 9 commits August 19, 2021 14:43

from locobot to default

53b8ca6

Merge branch 'allenai:main' into default-agent-ithor

dfb5641

Objectnav iTHOR default agent baseline

877f7a5

Update object_nav_ithor_ppo_baseline.py

0d33b2b

added args to ithor controller

66a5b03

dataset task sampler for ithor objectnav default

668b3ca

updated model to ResnetGRU

7d8a136

Merge branch 'allenai:main' into default-agent-ithor

aadc4a1

autoformatted ithorObjectnav default agentmode

24fb1d1

kshitijd20 requested a review from Lucaweihs August 24, 2021 16:15

jordis-ai2 reviewed Aug 24, 2021

View reviewed changes

reset to default iTHOR args

37795be

kshitijd20 added 2 commits August 24, 2021 22:22

restore iTHOR defaults

6a72649

looping in val/test sampler to False

90ef4f5

fixed typos

a7c7f0d

kshitijd20 marked this pull request as draft August 24, 2021 21:59

Lucaweihs reviewed Aug 25, 2021

View reviewed changes

Kshitij Dwivedi added 2 commits August 25, 2021 06:14

Dataset Task sampler naming change

6a07db0

pose to position

fe3ed2e

adding reward config

be37e2a

Kshitij Dwivedi added 2 commits August 25, 2021 14:10

shaping rewards added

a76d94f

auto formatting

11bc7cd

LGTM warnings addressed

5f86224

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding default agentmode for ithor objectnav task #307

Adding default agentmode for ithor objectnav task #307

kshitijd20 commented Aug 24, 2021 •

edited

lgtm-com bot commented Aug 24, 2021

jordis-ai2 left a comment

jordis-ai2 Aug 24, 2021

kshitijd20 Aug 24, 2021

jordis-ai2 Aug 24, 2021

kshitijd20 Aug 24, 2021

jordis-ai2 Aug 25, 2021

lgtm-com bot commented Aug 24, 2021

lgtm-com bot commented Aug 24, 2021

lgtm-com bot commented Aug 24, 2021

Lucaweihs left a comment

Lucaweihs Aug 24, 2021

kshitijd20 Aug 25, 2021

Lucaweihs Aug 26, 2021

Lucaweihs Aug 24, 2021

Lucaweihs Aug 24, 2021

jordis-ai2 Aug 25, 2021

kshitijd20 Aug 25, 2021

Lucaweihs Aug 26, 2021

jordis-ai2 Aug 26, 2021

lgtm-com bot commented Aug 25, 2021

lgtm-com bot commented Aug 25, 2021

lgtm-com bot commented Aug 25, 2021

Adding default agentmode for ithor objectnav task #307

Are you sure you want to change the base?

Adding default agentmode for ithor objectnav task #307

Conversation

kshitijd20 commented Aug 24, 2021 • edited

lgtm-com bot commented Aug 24, 2021

jordis-ai2 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

lgtm-com bot commented Aug 24, 2021

lgtm-com bot commented Aug 24, 2021

lgtm-com bot commented Aug 24, 2021

Lucaweihs 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

lgtm-com bot commented Aug 25, 2021

lgtm-com bot commented Aug 25, 2021

lgtm-com bot commented Aug 25, 2021

kshitijd20 commented Aug 24, 2021 •

edited