-
Notifications
You must be signed in to change notification settings - Fork 53
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
Adding default agentmode for ithor objectnav task #307
base: main
Are you sure you want to change the base?
Conversation
This pull request introduces 6 alerts when merging 24fb1d1 into 0978361 - view on LGTM.com new alerts:
|
There was a problem hiding this 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 👍
@@ -38,11 +38,16 @@ def __init__( | |||
fov: float = FOV, | |||
player_screen_width: int = 300, | |||
player_screen_height: int = 300, | |||
gridSize: float = 0.15, |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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-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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks!
This pull request introduces 6 alerts when merging 37795be into 0978361 - view on LGTM.com new alerts:
|
This pull request introduces 6 alerts when merging 90ef4f5 into 0978361 - view on LGTM.com new alerts:
|
This pull request introduces 6 alerts when merging a7c7f0d into 0978361 - view on LGTM.com new alerts:
|
There was a problem hiding this 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!
def reset_object_filter(self): | ||
self.controller.step("ResetObjectFilter", renderImage=False) | ||
|
||
def teleport( |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
@@ -198,3 +200,218 @@ def set_seed(self, seed: int): | |||
self.seed = seed | |||
if seed is not None: | |||
set_seed(seed) | |||
|
|||
|
|||
class ObjectNavDatasetTaskSampler(TaskSampler): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we call this ObjectNaviThorDatasetTaskSampler
? I'd also be up for renaming the existing ObjectNavTaskSampler
class in this file to ObjectNaviThorTaskSampler
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 👍
This pull request introduces 6 alerts when merging fe3ed2e into 0978361 - view on LGTM.com new alerts:
|
This pull request introduces 7 alerts when merging be37e2a into 0978361 - view on LGTM.com new alerts:
|
This pull request introduces 7 alerts when merging 11bc7cd into 0978361 - view on LGTM.com new alerts:
|
Added config to train objectnav baseline in iTHOR using
default
agentmode. The following files were updatedallenact/projects/tutorials/object_nav_ithor_ppo_baseline.py
contains the experiment configallenact/allenact_plugins/ithor_plugin/ithor_environment.py
added environment arguments e.g. rotatestepdegrees and snaptogrid to pass to the controllerallenact/allenact_plugins/ithor_plugin/ithor_task_samplers.py
added objectnavdataset sampler