Skip to content
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

Self-supervised Aux Tasks in pointnav and objectnav #319

Merged
merged 22 commits into from
Dec 17, 2021

Conversation

twni2016
Copy link
Contributor

@twni2016 twni2016 commented Dec 1, 2021

This PR includes:

  • Provide logging in one folder option and make it as default
  • Provide a base class VisualNavActorCritic to be inherited from different downstream navigation model classes (i.e. code refactoring), and also add the modified ResNet18 from https://github.com/joel99/habitat-pointnav-aux
  • Introduce self-supervised aux tasks and fusion models based on the code: https://github.com/joel99/habitat-pointnav-aux , including inverse dynamics, temporal distance, and CPC|A.
  • Update corresponding experiment config files in pointnav and objectnav

I tested the change by running commands:

python main.py projects/objectnav_baselines/experiments/robothor/objectnav_robothor_rgb_rawgru_ddppo.py \
    -o experiment_output/objectnav/ --seed 302
python main.py projects/pointnav_baselines/experiments/robothor/pointnav_robothor_rgb_simpleconvgru_ddppo.py\
    -o experiment_output/pointnav/ --seed 302

@twni2016 twni2016 changed the title [WIP] Self-supervised Aux Tasks Self-supervised Aux Tasks in pointnav and objectnav Dec 2, 2021
@twni2016 twni2016 requested a review from Lucaweihs December 2, 2021 05:00
@lgtm-com
Copy link

lgtm-com bot commented Dec 2, 2021

This pull request introduces 11 alerts when merging 4a4fd5f into 894c2ce - view on LGTM.com

new alerts:

  • 9 for Unused import
  • 2 for Unused local variable

Copy link
Collaborator

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

Awesome work Tianwei! I'm always impressed when I see the amount of work you put in and the code you produced. I've left a lot of comments but they are mostly about naming and adding doc strings. One thing: there were a few instances where a change broke backwards compatibility/behavior. In general we really try to prevent this as much as possible so it would be great to find solutions that make new behavior opt-in rather than opt-out (if you feel strongly about making a new behavior the new default then please add an argument to this thread).

allenact/algorithms/onpolicy_sync/losses/ppo.py Outdated Show resolved Hide resolved
allenact/algorithms/onpolicy_sync/runner.py Outdated Show resolved Hide resolved
allenact/algorithms/onpolicy_sync/runner.py Outdated Show resolved Hide resolved
allenact/algorithms/onpolicy_sync/runner.py Outdated Show resolved Hide resolved
allenact/algorithms/onpolicy_sync/runner.py Outdated Show resolved Hide resolved
projects/objectnav_baselines/models/object_nav_models.py Outdated Show resolved Hide resolved
projects/objectnav_baselines/models/object_nav_models.py Outdated Show resolved Hide resolved
@lgtm-com
Copy link

lgtm-com bot commented Dec 12, 2021

This pull request introduces 13 alerts when merging fee8003 into eb2c12e - view on LGTM.com

new alerts:

  • 11 for Unused import
  • 2 for Unused local variable

@twni2016
Copy link
Contributor Author

twni2016 commented Dec 12, 2021

Most of comments have been resolved. I've refactored the logging by using save_dir_fmt argument.
Remaining issues:

@twni2016
Copy link
Contributor Author

I use the following script for testing:

python main.py projects/objectnav_baselines/experiments/robothor/objectnav_robothor_rgb_unfrozenresnet_gru_ddppo.py \
    -s 302 --save_dir_fmt NESTED \
    -o experiment_output/objectnav/
python main.py projects/pointnav_baselines/experiments/robothor/pointnav_robothor_depth_simpleconvgru_ddppo.py \
    -s 302 --save_dir_fmt NESTED \
    -o experiment_output/pointnav/ 

Shall we also add these running scripts into somewhere in the doc?

@lgtm-com
Copy link

lgtm-com bot commented Dec 12, 2021

This pull request introduces 13 alerts when merging 8479e03 into eb2c12e - view on LGTM.com

new alerts:

  • 11 for Unused import
  • 2 for Unused local variable

Copy link
Collaborator

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

Great updates! A few small little things to clean up left but almost there.

allenact/algorithms/onpolicy_sync/runner.py Outdated Show resolved Hide resolved
allenact/algorithms/onpolicy_sync/runner.py Outdated Show resolved Hide resolved
allenact/algorithms/onpolicy_sync/runner.py Show resolved Hide resolved
allenact/algorithms/onpolicy_sync/runner.py Outdated Show resolved Hide resolved
allenact/algorithms/onpolicy_sync/runner.py Outdated Show resolved Hide resolved
allenact/utils/model_utils.py Outdated Show resolved Hide resolved
@lgtm-com
Copy link

lgtm-com bot commented Dec 14, 2021

This pull request introduces 13 alerts when merging 21bf39d into eb2c12e - view on LGTM.com

new alerts:

  • 11 for Unused import
  • 2 for Unused local variable

@twni2016
Copy link
Contributor Author

These LGTM alerts are not coming from my code. Shall I resolve them? @Lucaweihs

@Lucaweihs
Copy link
Collaborator

@twni2016 - We might be looking at different LGTM pages but you can find some alerts in your new files here. These are mostly about unused imports but there also appear to be 2 instances of unused local variables.

@twni2016
Copy link
Contributor Author

I see. I clicked the alert button in LGTM webpage which shows other lines.

@lgtm-com
Copy link

lgtm-com bot commented Dec 14, 2021

This pull request introduces 13 alerts when merging 074e5ac into eb2c12e - view on LGTM.com

new alerts:

  • 11 for Unused import
  • 2 for Unused local variable

@twni2016
Copy link
Contributor Author

twni2016 commented Dec 15, 2021

@Lucaweihs I have fixed the key mismatch issue in objectnav checkpoint. The script

python main.py projects/objectnav_baselines/experiments/robothor/objectnav_robothor_rgbd_resnetgru_ddppo.py \
    -s 1 --save_dir_fmt NESTED --infer_output_dir --eval -i \
    -c pretrained_model_ckpts/robothor-objectnav-challenge-2021/Objectnav-RoboTHOR-RGBD-ResNetGRU-DDPPO/2021-02-09_22-35-15/exp_Objectnav-RoboTHOR-RGBD-ResNetGRU-DDPPO_0.2.0a_300M__stage_00__steps_000170207237.pt

But, I cannot find the experiment config robothor-pointnav-rgb-resnet for pointnav checkpoint, so I cannot run it. By the way, the wget script pretrained_model_ckpts/download_navigation_model_ckpts.sh download the ckpt into pretrained_model_ckpts for objectnav while into the root dir for pointnav. I think they should be consistent.

@lgtm-com
Copy link

lgtm-com bot commented Dec 15, 2021

This pull request introduces 5 alerts when merging 2d8c480 into eb2c12e - view on LGTM.com

new alerts:

  • 3 for Unused import
  • 2 for Unused local variable

Copy link
Collaborator

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

There are a few straggling LGTM.com alerts. I've highlighted them here. They should be quick fixes, we're good to merge after these are in.

allenact/embodiedai/aux_losses/losses.py Outdated Show resolved Hide resolved
allenact/embodiedai/aux_losses/losses.py Outdated Show resolved Hide resolved
allenact/embodiedai/aux_losses/losses.py Outdated Show resolved Hide resolved
@lgtm-com
Copy link

lgtm-com bot commented Dec 16, 2021

This pull request introduces 2 alerts when merging 387d9d8 into eb2c12e - view on LGTM.com

new alerts:

  • 2 for Unused import

@Lucaweihs Lucaweihs merged commit 348a30d into main Dec 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants