Added center cropping and resize ops for PPO agents #365

Skylion007 · 2020-04-07T23:33:47Z

Motivation and Context

Allows people to train PPO agents with non-square screen resolution. Add config options to automatically resize and crop the image in a performant manner.

Feedback requested on how to best implement this API and configs.

How Has This Been Tested

It has been tested by testing the utility functions on various local agents.

Types of changes

Docs change / refactoring / dependency upgrade
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

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.

codecov · 2020-04-08T00:15:18Z

Codecov Report

Merging #365 into master will increase coverage by 0.03%.
The diff coverage is 84.82%.

@@            Coverage Diff             @@
##           master     #365      +/-   ##
==========================================
+ Coverage   77.24%   77.27%   +0.03%     
==========================================
  Files         108      108              
  Lines        7330     7425      +95     
==========================================
+ Hits         5662     5738      +76     
- Misses       1668     1687      +19

Impacted Files	Coverage Δ
habitat_baselines/agents/ppo_agents.py	`76.00% <ø> (ø)`
habitat_baselines/config/default.py	`98.03% <75.00%> (-0.95%)`	⬇️
habitat_baselines/common/utils.py	`79.10% <77.77%> (-2.15%)`	⬇️
habitat_baselines/rl/ddppo/policy/resnet_policy.py	`90.65% <100.00%> (+0.55%)`	⬆️
habitat_baselines/rl/models/simple_cnn.py	`100.00% <100.00%> (ø)`
test/test_baseline_agents.py	`91.66% <100.00%> (+1.92%)`	⬆️
habitat_baselines/common/environments.py	`97.72% <0.00%> (-2.28%)`	⬇️
habitat/tasks/nav/nav.py	`94.16% <0.00%> (-0.25%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 676e593...d495ee2. Read the comment docs.

habitat_baselines/common/utils.py

habitat_baselines/rl/ppo/ppo_trainer.py

…d config system.

habitat_baselines/rl/ddppo/policy/resnet_policy.py

mathfac · 2020-04-11T17:28:27Z

habitat_baselines/config/default.py

@@ -144,7 +144,10 @@ def get_config(

        for config_path in config_paths:
            config.merge_from_file(config_path)
-
+    if opts:
+        for k, v in zip(opts[0::2], opts[1::2]):


What is the logic behind this change?

It's impossible to overwrite the BASE_TASK_CONFIG from the command line without since the BASE_TASK_CONFIG is used before its args are overwritten by the command line. Likewise, moving the code for that to this point would make it impossible to overwrite TASK_CONFIG variables from the command line. As such, BASE_TASK_CONFIG must be extracted and overwritten and then the remaining config parameters can be overwritten.

mathfac · 2020-04-11T17:38:34Z

habitat_baselines/common/utils.py

@@ -53,6 +56,51 @@ def forward(self, x):
        return CustomFixedCategorical(logits=x)


+class ResizeCenterCropper(nn.Module):
+    def __init__(
+        self, force_input_size: Optional[tuple], channels_first: bool = True


force_input_size name is a little bit confusing. Is that input size for transformation or size of input to the next level. Maybe, use naming similar to [torchvision.transforms.CenterCrop]:(https://pytorch.org/docs/stable/torchvision/transforms.html#torchvision.transforms.CenterCrop)
size (sequence or python:int) – Desired output size of the crop. If size is an int instead of sequence like (h, w), a square crop (size, size) is made.

Regarding channels_first maybe call it like NCHW and add docstring. Because, channels are not really first and to be more explicit.

It also resizes the input the though, it doesn't just center crop. I also kept it like so because at least for center cropping I wrote the code such that ....CHW or ...HWC (regardless of the number of channels) were supported. The channels_first is a convention from Tensorflow/Keras.

I changed to channels_list since that is true regardless of the channels number.

mathfac · 2020-04-11T17:39:03Z

habitat_baselines/common/utils.py

+        # NHWC
+        h, w = img.shape[-3:-1]
+        if len(img.shape) == 4:
+            img = img.permute(0, 3, 1, 2)


Why do we need to do this permutations?

Because PyTorch only accepts NCHW channel order for that function.

@Skylion007, for each img.permute can we add inline comments like # NHWC = >NCHW. Then it will be easier to support the code.

habitat_baselines/common/utils.py

mathfac · 2020-04-11T17:43:58Z

habitat_baselines/common/utils.py

+    return observations
+
+
+def overwrite_gym_box(box: Box, shape: tuple) -> Box:


Maybe overwrite_gym_box_height_width.

It can also overwrite channels if specified though or any other aspects of shape.

habitat_baselines/common/utils.py

mathfac · 2020-04-11T17:48:53Z

habitat_baselines/common/utils.py

+            return input
+
+        return center_crop(
+            image_resize_shortest_edge(


How to disable image_resize_shortest_edge functionality in current setup?

There isn't a way, that's why it's ResizeCenterCropper not just CenterCropper. Currently there is no way to disable it.

mathfac · 2020-04-11T17:50:41Z

test/test_baseline_agents.py

-
-        agent = ppo_agents.PPOAgent(agent_config)
-        habitat.logger.info(benchmark.evaluate(agent, num_episodes=10))
+        for resolution in [256, 384]:


Use resolution for @pytest.mark.parametrize like here.

That would require reconstructing the agent and benchmark for every iteration? I think there is a reason this is already done in a loop before I added the code (it would make the test a lot longer).

Makes sense, do you want to test when h <> w?

mathfac

Requested some changes, please add doc strings as well to ResizeCenterCropper.

mathfac · 2020-04-13T18:44:47Z

habitat_baselines/common/utils.py

@@ -174,3 +226,92 @@ def generate_video(
        tb_writer.add_video_from_np_images(
            f"episode{episode_id}", checkpoint_idx, images, fps=fps
        )
+
+
+def image_resize_shortest_edge(img, size: int, channels_last: bool = False):


Please, specify type for img.

Suggested change

def image_resize_shortest_edge(img, size: int, channels_last: bool = False):

def image_resize_shortest_edge(img: torch.Tensor, size: int, channels_last: bool = False):

habitat_baselines/common/utils.py

mathfac

@Skylion007 thank you for following on all the comments. Looks much better now. Left some suggestions.
Quick question: do you have a sense of benchmark how performance will be affected if the resize, crop is enabled?
Thank you!

Skylion007 · 2020-04-13T19:01:20Z

@mathfac it's not noticeably affected and it shouldn't be as all those operations are performed on the GPU. (Cropping is a straight up array view and resizing uses an optimized GPU kernel to compute it.

mathfac · 2020-04-13T19:02:18Z

@Skylion007, looks like habitat baseline config part is missing to configure ResizeCrop.

Skylion007 · 2020-04-13T19:31:24Z

@Skylion007, looks like habitat baseline config part is missing to configure ResizeCrop.

We don't even have a good way to deserialize arbitrary transforms for this yet and walking the config down to that scope would require significant code changes at the moment so I plan on leaving it hard coded for now.

mathfac · 2020-04-15T15:58:25Z

Tested performance and functionality for ObjectNav
For 415232 frames:
With resize Depth only 640x480: env-time: 2513.411s pth-time: 3503.996s
Without resize Depth only 1x1: env-time: 1744.576s pth-time: 3602.428s
+44% env-time. I think it acceptable with resolution change. Merging.

* --cov-append for second pytest Add coverage flags in just the CI, not globally * Add tests without CUDA

…#365) Allows people to train PPO agents with non-square screen resolution. Add config options to automatically resize and crop the image in a performant manner.

Added center cropping and resize ops

bd98e86

facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Apr 7, 2020

Fixed typos

1689a12

erikwijmans reviewed Apr 8, 2020

View reviewed changes

habitat_baselines/common/utils.py Outdated Show resolved Hide resolved

Used torch interpolate and added more tests

c8df9d7

erikwijmans reviewed Apr 8, 2020

View reviewed changes

habitat_baselines/common/utils.py Outdated Show resolved Hide resolved

Added center_crop and resize to the trainers

32f25ed

Skylion007 changed the title ~~[WIP] Added center cropping and resize ops for PPO agents~~ Added center cropping and resize ops for PPO agents Apr 8, 2020

erikwijmans approved these changes Apr 8, 2020

View reviewed changes

habitat_baselines/rl/ppo/ppo_trainer.py Outdated Show resolved Hide resolved

mathfac mentioned this pull request Apr 8, 2020

Observation transformation registry for Habitat baselines to provide input cropping, resizing, etc. #367

Closed

Skylion007 added 2 commits April 9, 2020 11:07

Overhauled API. Hardcoded 256x256 for habitat_baselines for now. Fixe…

0884a8d

…d config system.

Cleaned up old code

7518d05

erikwijmans reviewed Apr 9, 2020

View reviewed changes

habitat_baselines/rl/ddppo/policy/resnet_policy.py Outdated Show resolved Hide resolved

Refeactored into GenericTransform API

1988d8d

mathfac reviewed Apr 11, 2020

View reviewed changes

habitat_baselines/common/utils.py Outdated Show resolved Hide resolved

mathfac reviewed Apr 11, 2020

View reviewed changes

habitat_baselines/common/utils.py Outdated Show resolved Hide resolved

mathfac reviewed Apr 11, 2020

View reviewed changes

habitat_baselines/common/utils.py Outdated Show resolved Hide resolved

mathfac reviewed Apr 11, 2020

View reviewed changes

habitat_baselines/common/utils.py Show resolved Hide resolved

mathfac reviewed Apr 11, 2020

View reviewed changes

habitat_baselines/common/utils.py Outdated Show resolved Hide resolved

mathfac reviewed Apr 11, 2020

View reviewed changes

habitat_baselines/common/utils.py Outdated Show resolved Hide resolved

mathfac reviewed Apr 11, 2020

View reviewed changes

habitat_baselines/common/utils.py Outdated Show resolved Hide resolved

mathfac reviewed Apr 11, 2020

View reviewed changes

mathfac requested changes Apr 11, 2020

View reviewed changes

Updated the API

514ee1b

mathfac reviewed Apr 13, 2020

View reviewed changes

habitat_baselines/common/utils.py Outdated Show resolved Hide resolved

mathfac reviewed Apr 13, 2020

View reviewed changes

habitat_baselines/common/utils.py Outdated Show resolved Hide resolved

mathfac approved these changes Apr 13, 2020

View reviewed changes

Skylion007 added 2 commits April 13, 2020 12:38

style changes

27ad573

Added additional comments

d495ee2

mathfac merged commit 959bd45 into master Apr 15, 2020

mathfac deleted the resize-center-ppo branch April 15, 2020 15:59

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

--cov-append for second pytest (#365)

a710809

* --cov-append for second pytest Add coverage flags in just the CI, not globally * Add tests without CUDA

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added center cropping and resize ops for PPO agents #365

Added center cropping and resize ops for PPO agents #365

Skylion007 commented Apr 7, 2020 •

edited

Loading

codecov bot commented Apr 8, 2020 •

edited

Loading

mathfac Apr 11, 2020

Skylion007 Apr 12, 2020

mathfac Apr 11, 2020

Skylion007 Apr 12, 2020

Skylion007 Apr 13, 2020

mathfac Apr 11, 2020

Skylion007 Apr 12, 2020

mathfac Apr 13, 2020

mathfac Apr 11, 2020

Skylion007 Apr 12, 2020

mathfac Apr 11, 2020

Skylion007 Apr 12, 2020

mathfac Apr 11, 2020

Skylion007 Apr 12, 2020

mathfac Apr 13, 2020

mathfac left a comment

mathfac Apr 13, 2020

mathfac left a comment

Skylion007 commented Apr 13, 2020

mathfac commented Apr 13, 2020

Skylion007 commented Apr 13, 2020

mathfac commented Apr 15, 2020 •

edited

Loading

		return observations


		def overwrite_gym_box(box: Box, shape: tuple) -> Box:

	def image_resize_shortest_edge(img, size: int, channels_last: bool = False):
	def image_resize_shortest_edge(img: torch.Tensor, size: int, channels_last: bool = False):

Added center cropping and resize ops for PPO agents #365

Added center cropping and resize ops for PPO agents #365

Conversation

Skylion007 commented Apr 7, 2020 • edited Loading

Motivation and Context

How Has This Been Tested

Types of changes

Checklist

codecov bot commented Apr 8, 2020 • edited Loading

Codecov Report

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

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

mathfac left a comment

Choose a reason for hiding this comment

Skylion007 commented Apr 13, 2020

mathfac commented Apr 13, 2020

Skylion007 commented Apr 13, 2020

mathfac commented Apr 15, 2020 • edited Loading

Skylion007 commented Apr 7, 2020 •

edited

Loading

codecov bot commented Apr 8, 2020 •

edited

Loading

mathfac commented Apr 15, 2020 •

edited

Loading