VLN Task & Dataset with shortest path example and tests. #233

koshyanand · 2019-10-22T11:56:37Z

Motivation and Context

Add VLN Task
Add reading of Room-to-Room Dataset
Add VLN Instruction Sensor
Added shortest path follower
Added benchmark for Random, ForwardOnly, RandomForward & GoalFollower agent
Represent instructions as list of tokens
Added vocabulary API to R2R dataset

How Has This Been Tested

Config test
Dataset size and serialization test
Instruction Sensor test & test to see if the sensor instruction matches the current instruction

Types of changes

Checklist

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have completed my CLA (see CONTRIBUTING)
I have added tests to cover my changes.
All new and existing tests passed.

…each episode

…ction. Assumes dataset episodes have 1 instruction and 1 trajectory

erikwijmans

Some initial comments. Will dive in more later.

habitat/tasks/vln/vln.py

habitat/tasks/utils.py

configs/tasks/vln_r2r.yaml

…ample.

mathfac

Awesome progress! VLN task is a one of the most anticipated tasks in Habitat. Left a part of the comments.

mathfac · 2019-10-26T00:04:48Z

configs/datasets/vln/mp3d_r2r.yaml

+DATASET:
+  TYPE: R2RVLN-v1
+  SPLIT: train
+  DATA_PATH: data/R2R/habitat_R2R_{split}.json


To follow naming dataset files naming convention I would advice next naming:

Suggested change

DATA_PATH: data/R2R/habitat_R2R_{split}.json

DATA_PATH: data/datasets/r2r/{split}/{split}.json.gz

mathfac · 2019-10-26T00:06:35Z

configs/tasks/vln_r2r.yaml

+  HABITAT_SIM_V0:
+    GPU_DEVICE_ID: 0
+  RGB_SENSOR:
+    WIDTH: 512


Is that expected that RGB resolution is different from Depth resolution?

mathfac · 2019-10-26T00:07:49Z

configs/datasets/vln/mp3d_r2r.yaml

+  TYPE: R2RVLN-v1
+  SPLIT: train
+  DATA_PATH: data/R2R/habitat_R2R_{split}.json
+  SCENES_DIR: data/mp3d/scenes/


Default path is expected to be:

Suggested change

SCENES_DIR: data/mp3d/scenes/

SCENES_DIR: "data/scene_datasets/"

mathfac · 2019-10-26T00:08:19Z

configs/tasks/vln_r2r.yaml

+  TYPE: R2RVLN-v1
+  SPLIT: train
+  DATA_PATH: data/datasets/R2R/hb_R2R_{split}.json
+  SCENES_DIR: data/scene_datasets/mp3d/


Same comment regarding dataset paths.

configs/tasks/vln_r2r.yaml

mathfac · 2019-10-26T00:10:18Z

configs/tasks/vln_r2r.yaml

+    HEIGHT: 256
+TASK:
+  TYPE: VLN-v0
+  SUCCESS_DISTANCE: 2.0


2 meters are acceptable success distance? Can be pretty large threshold.

configs/tasks/vln_r2r.yaml

mathfac · 2019-10-26T00:12:23Z

configs/test/habitat_r2r_vln_test.yaml

+DATASET:
+  TYPE: R2RVLN-v1
+  SPLIT: val_seen
+  DATA_PATH: data/datasets/R2R/hb_R2R_{split}.json


Dataset files paths and naming.

jacobkrantz · 2019-10-27T05:31:38Z

configs/tasks/vln_r2r.yaml

+    HEIGHT: 256
+TASK:
+  TYPE: VLN-v0
+  SUCCESS_DISTANCE: 3.0


The success distance is set to 3m here which is the distance defined in the original VLN paper and is used by most papers in this space.

mathfac

You are really close, left some final comments. Is it possible to add link to data download into README? Thank you!

mathfac · 2019-10-28T06:31:05Z

configs/tasks/vln_r2r.yaml

+  TYPE: VLN-v0
+  SUCCESS_DISTANCE: 3.0
+  SENSORS: ['INSTRUCTION_SENSOR']
+  POSSIBLE_ACTIONS: ['MOVE_FORWARD', 'TURN_LEFT', 'TURN_RIGHT', 'STOP']


Maybe worth to make 'STOP' 0 action, as list of other actions can expand.

mathfac · 2019-10-30T09:12:09Z

test/test_r2r_vln.py

+        dataset_config
+    ):
+        pytest.skip(
+            "Please download Matterport3D R2R dataset to " "data folder."


Suggested change

"Please download Matterport3D R2R dataset to " "data folder."

"Please download Matterport3D R2R dataset to data folder."

test/test_r2r_vln.py

mathfac · 2019-10-30T09:13:25Z

test/test_r2r_vln.py

+    follower = ShortestPathFollower(
+        env.habitat_env.sim, goal_radius=0.5, return_one_hot=False
+    )
+


Suggested change

mathfac · 2019-10-30T09:15:00Z

test/test_r2r_vln.py

@@ -0,0 +1,120 @@
+#!/usr/bin/env python3


Thank you for adding relevant tests.

mathfac · 2019-10-30T09:21:07Z

habitat/tasks/vln/vln.py

+        start_rotation: numpy ndarray with 4 entries for (x, y, z, w)
+            elements of unit quaternion (versor) representing agent 3D
+            orientation.
+        instruction: single instruction guide to goal.


Suggested change

instruction: single instruction guide to goal.

instruction: single natural language instruction guide to goal.

mathfac · 2019-10-30T09:22:14Z

habitat/tasks/vln/vln.py

+
+
+@registry.register_task(name="VLN-v0")
+class VLNTask(NavigationTask):


Extensive doc string should be added with concise description of the task.

mathfac · 2019-10-30T09:24:04Z

habitat/datasets/vln/r2r_vln_dataset.py

+CONTENT_SCENES_PATH_FIELD = "content_scenes_path"
+DEFAULT_SCENE_PATH_PREFIX = "data/scene_datasets/mp3d/"
+
+R2R_TRAIN_EPISODES = 10837


This constants are never used.

mathfac · 2019-10-30T09:27:35Z

examples/vln_shortest_path_follower_example.py

+        output_im, observations["instruction"]["text"]
+    )
+    images.append(output_im)
+


Are all functions above required? Can we import and reuse functions from other examples or util functions?

mathfac · 2019-10-30T09:31:47Z

examples/vln_benchmark.py

+    return avg_metrics
+
+
+def main():


Ideally, we would like to have same launcher for different task benchmarks. It should be possible to incorporate task config into examples/benchmark.py and make it support VLN as well. But most probably one more registry needed for supported agents/baselines. Let's create follow up issue with merging benchmark.py and vln_benchmark.py.

mathfac

Great job @koshyanand and @jacobkrantz, really like how you figured out a lot details from existing code. Some minor comments otherwise ready to merge.

mathfac · 2019-10-31T06:44:04Z

examples/vln_shortest_path_follower_example.py

+    os.makedirs(IMAGE_DIR)
+
+
+def append_text_to_image(orig_img, text):


Maybe move to habitat.utils.visualizations.utils.

mathfac · 2019-10-31T06:47:52Z

habitat/datasets/vln/r2r_vln_dataset.py

+
+        deserialized = json.loads(json_str)
+
+        # Done for the serialization test


What is the reason that we need this if? Why we don't have deserialized["instruction_vocab"]["word_list"] in released version of dataset?

mathfac · 2019-10-31T06:49:04Z

habitat/tasks/vln/vln.py

+        trajectory_id: id of ground truth trajectory path.
+        goals: relevant goal object/room.
+    """
+    path: List[List[float]] = attr.ib(


Make the rotation in ShortestPathPoint optional sounds good. You can re-use visualization we have for shortest path already.

mathfac · 2019-11-04T19:54:05Z

habitat/tasks/vln/vln.py

@@ -0,0 +1,92 @@
+#!/usr/bin/env python3


You can include your names as owners/maintainers of the VLN task here as well as reflect your contribution in README section.

I added our names in this file. For the README section, we can wait until we have a paper (as per discussion).

erikwijmans · 2019-11-21T23:36:45Z

configs/tasks/vln_r2r.yaml

+  RGB_SENSOR:
+    WIDTH: 256
+    HEIGHT: 256
+    HFOV: 45


Why 45 degrees? That seems incredibly narrow

mathfac · 2019-11-22T19:06:20Z

configs/datasets/vln/mp3d_r2r.yaml

+DATASET:
+  TYPE: R2RVLN-v1
+  SPLIT: train
+  DATA_PATH: "data/datasets/vln/r2r/v1/{split}/{split}.json.gz"


To be consistent with our dataset paths let change path to everywhere:

Suggested change

DATA_PATH: "data/datasets/vln/r2r/v1/{split}/{split}.json.gz"

DATA_PATH: "data/datasets/vln/mp3d/r2r/v1/{split}/{split}.json.gz"

mathfac · 2019-11-22T19:08:36Z

habitat/datasets/vln/r2r_vln_dataset.py

+R2R_VAL_SEEN_EPISODES = 781
+
+
+@registry.register_dataset(name="R2RVLN-v1")


We should register it MP3DR2RVLN-v1 or change doc string not attach it to MP3D only. From my experience this class can be scene dataset agnostic.

Suggested change

@registry.register_dataset(name="R2RVLN-v1")

@registry.register_dataset(name="R2RVLN-v1")

I changed the doc string as the class is not tied to MP3D.

jacobkrantz · 2019-11-24T06:55:20Z

@erikwijmans @mathfac back to you!

mathfac · 2019-12-05T19:55:31Z

@jacobkrantz and @koshyanand thank you for the contribution of such popular Embodied AI task and following all the comments.

This test is randomly failing (#233). Try to make it deterministic so that it always fails or always passes.

…earch#233) * Add VLN Task * Add reading of Room-to-Room Dataset * Add VLN Instruction Sensor * Added shortest path follower * Added benchmark for Random, ForwardOnly, RandomForward & GoalFollower agent * Represent instructions as list of tokens * Added vocabulary API to R2R dataset

koshyanand and others added 11 commits September 12, 2019 21:24

Copied over all changes from habitat-fork

44ad8ac

Add Benchmarking for simple agents and a goal follower example

5fba9d3

Merge remote-tracking branch 'upstream/master'

ab92fd6

Implement InstructionSensor

37be6b4

rm a print

65303f8

Instruction added to goal & only a single instruction is loaded with …

9d5e5dc

…each episode

Added a few test cases

8ac8d0f

Update instruction sensor to return trajectory_id and a single instru…

2d4f9ec

…ction. Assumes dataset episodes have 1 instruction and 1 trajectory

Implement tokenization for VLN instructions

212a6b6

Added test cases for verifying instruction from sensor

1aaae7d

Code style enforced & minor changes

39a6b8b

facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Oct 22, 2019

erikwijmans requested review from mathfac, abhiskk and erikwijmans and removed request for mathfac October 22, 2019 15:17

erikwijmans reviewed Oct 22, 2019

View reviewed changes

habitat/tasks/vln/vln.py Outdated Show resolved Hide resolved

habitat/tasks/utils.py Outdated Show resolved Hide resolved

configs/tasks/vln_r2r.yaml Outdated Show resolved Hide resolved

jacobkrantz and others added 3 commits October 22, 2019 15:46

Update VLN configs, styling

720979a

Added instructions at the bottom of the image in the path follower ex…

6602cbf

…ample.

Styling

345679e

koshyanand requested a review from erikwijmans October 24, 2019 03:52

mathfac reviewed Oct 26, 2019

View reviewed changes

Config and data filename updates

41ef6f7

jacobkrantz reviewed Oct 27, 2019

View reviewed changes

jacobkrantz added 2 commits October 28, 2019 10:17

Merge branch 'master' of https://github.com/facebookresearch/habitat-api

b24891e

possible_actions ordering

628f2d5

mathfac reviewed Oct 30, 2019

View reviewed changes

koshyanand and others added 2 commits October 30, 2019 05:08

Code refactoring, documenting and cleaning

8c7ab5c

Docstrings and instruction sensor UUID

14f9eb7

mathfac approved these changes Oct 31, 2019

View reviewed changes

Move helper to utils, simplify dataset loading

64d2215

jacobkrantz force-pushed the master branch from b2e9e48 to 64d2215 Compare October 31, 2019 18:58

mathfac reviewed Nov 4, 2019

View reviewed changes

erikwijmans reviewed Nov 21, 2019

View reviewed changes

Merge branch 'master' of https://github.com/facebookresearch/habitat-api

f13a8f4

mathfac reviewed Nov 22, 2019

View reviewed changes

Address review, fix vln_benchmark

a3bb292

jacobkrantz force-pushed the master branch from d7a2a09 to a3bb292 Compare November 23, 2019 07:01

Add method get_scenes_to_load

9c9d8df

jacobkrantz force-pushed the master branch from f80466a to 9c9d8df Compare November 24, 2019 06:27

mathfac merged commit 8726f0d into facebookresearch:master Dec 5, 2019

erikwijmans mentioned this pull request Dec 27, 2019

Geodesic distance return inf in many cases. facebookresearch/habitat-sim#405

Closed

dhruvbatra pushed a commit that referenced this pull request May 10, 2020

[tests] test_agent.py: make test_change_state more deterministic (#258)

2a5cef9

This test is randomly failing (#233). Try to make it deterministic so that it always fails or always passes.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VLN Task & Dataset with shortest path example and tests. #233

VLN Task & Dataset with shortest path example and tests. #233

koshyanand commented Oct 22, 2019 •

edited

Loading

erikwijmans left a comment

mathfac left a comment

mathfac Oct 26, 2019

mathfac Oct 26, 2019

mathfac Oct 26, 2019

mathfac Oct 26, 2019

mathfac Oct 26, 2019

mathfac Oct 26, 2019

jacobkrantz Oct 27, 2019 •

edited

Loading

mathfac left a comment

mathfac Oct 28, 2019

mathfac Oct 30, 2019

mathfac Oct 30, 2019

mathfac Oct 30, 2019

mathfac Oct 30, 2019

mathfac Oct 30, 2019

mathfac Oct 30, 2019

mathfac Oct 30, 2019

mathfac Oct 30, 2019

mathfac left a comment

mathfac Oct 31, 2019

mathfac Oct 31, 2019

mathfac Oct 31, 2019

mathfac Nov 4, 2019

jacobkrantz Nov 23, 2019

erikwijmans Nov 21, 2019

mathfac Nov 22, 2019

mathfac Nov 22, 2019

jacobkrantz Nov 23, 2019

jacobkrantz commented Nov 24, 2019

mathfac commented Dec 5, 2019

	DATA_PATH: data/R2R/habitat_R2R_{split}.json
	DATA_PATH: data/datasets/r2r/{split}/{split}.json.gz

	SCENES_DIR: data/mp3d/scenes/
	SCENES_DIR: "data/scene_datasets/"

	"Please download Matterport3D R2R dataset to " "data folder."
	"Please download Matterport3D R2R dataset to data folder."

	instruction: single instruction guide to goal.
	instruction: single natural language instruction guide to goal.



		@registry.register_task(name="VLN-v0")
		class VLNTask(NavigationTask):

		os.makedirs(IMAGE_DIR)


		def append_text_to_image(orig_img, text):


		deserialized = json.loads(json_str)

		# Done for the serialization test

	DATA_PATH: "data/datasets/vln/r2r/v1/{split}/{split}.json.gz"
	DATA_PATH: "data/datasets/vln/mp3d/r2r/v1/{split}/{split}.json.gz"

		R2R_VAL_SEEN_EPISODES = 781


		@registry.register_dataset(name="R2RVLN-v1")

VLN Task & Dataset with shortest path example and tests. #233

VLN Task & Dataset with shortest path example and tests. #233

Conversation

koshyanand commented Oct 22, 2019 • edited Loading

Motivation and Context

How Has This Been Tested

Types of changes

Checklist

erikwijmans left a comment

Choose a reason for hiding this comment

mathfac 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

jacobkrantz Oct 27, 2019 • edited Loading

Choose a reason for hiding this comment

mathfac 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

mathfac 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

jacobkrantz commented Nov 24, 2019

mathfac commented Dec 5, 2019

koshyanand commented Oct 22, 2019 •

edited

Loading

jacobkrantz Oct 27, 2019 •

edited

Loading