8000 Adding default agentmode for ithor objectnav task by kshitijd20 · Pull Request #307 · allenai/allenact · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Adding default agentmode for ithor objectnav task #307

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 19 commits into
base: main
Choose a base branch
from
Draft

Adding default agentmode for ithor objectnav task #307

wants to merge 19 commits into from

Conversation

kshitijd20
Copy link
@kshitijd20 kshitijd20 commented Aug 24, 2021 •
edited
Loading

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

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

@lgtm-com
Copy link
lgtm-com bot commented Aug 24, 2021

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

@kshitijd20 kshitijd20 requested a review from Lucaweihs August 24, 2021 16:15
jordis-ai2
jordis-ai2 reviewed Aug 24, 2021
View reviewed changes
Copy link
Collaborator
@jordis-ai2 jordis-ai2 left a comment

Choose a reason for hiding this comment

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

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 👍

allenact_plugins/ithor_plugin/ithor_environment.py Outdated
@@ -38,11 +38,16 @@ def __init__(
fov: float = FOV,
player_screen_width: int = 300,
player_screen_height: int = 300,
gridSize: float = 0.15,
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's actually 1.5:

https://ai2thor.allenai.org/ithor/documentation/initialization/#initialization-visibilitydista 8000 nce

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Great, thanks!

@lgtm-com
Copy link
lgtm-com bot commented Aug 24, 2021

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
Copy link
lgtm-com bot commented Aug 24, 2021

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

@kshitijd20 kshitijd20 marked this pull request as draft August 24, 2021 21:59
@lgtm-com
Copy link
lgtm-com bot commented Aug 24, 2021

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
Lucaweihs reviewed Aug 25, 2021
View reviewed changes
Copy link
Contributor
@Lucaweihs Lucaweihs left a comment

Choose a reason for hiding this comment

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

Some minor suggestions but this looks good to me!

allenact_plugins/ithor_plugin/ithor_environment.py
def reset_object_filter(self):
self.controller.step("ResetObjectFilter", renderImage=False)

def teleport(
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use the existing teleport_agent_to function?

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

allenact_plugins/ithor_plugin/ithor_task_samplers.py Outdated
@@ -198,3 +200,218 @@ def set_seed(self, seed: int):
self.seed = seed
if seed is not None:
set_seed(seed)


class ObjectNavDatasetTaskSampler(TaskSampler):
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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 👍

@lgtm-com
Copy link
lgtm-com bot commented Aug 25, 2021

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
Copy link
lgtm-com bot commented Aug 25, 2021

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
Copy link
lgtm-com bot commented Aug 25, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Lucaweihs Lucaweihs Lucaweihs left review comments

@jordis-ai2 jordis-ai2 jordis-ai2 left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kshitijd20 @Lucaweihs @jordis-ai2
0