Redwood noisy depth #189

erikwijmans · 2019-09-04T20:28:19Z

Motivation and Context

Adds the noise model from here: http://redwood-data.org/indoor/dataset.html

There are 2 implementations:

A numba based python implementation that runs at ~400 FPS at 256x256
A CUDA implementation that runs at ~5k FPS at 256x256 or ~200k FPS at 256x256 when using the GPU->GPU transfer from Sensors of different resolutions and GPU-GPU transfer #114 ! I am gonna call ~200k FPS as "fast enough".

@msavva @bigbike @mosra What are your thoughts on how I am currently providing arguments to the sensor noise models? I just added a py::dict to the python binding of the sensor spec so that people can pass arbitrary arguments in it.

TODO

Doc strings
License headers
Add citations
Figure out how to get the args for constructing noise models -- @mosra's suggestion to have a python only property seems to be a nice middle ground between PySensorSpec and just doing py::dynamic_attr()
Switch to a proper registry pattern in a different PR and use that here -- Registry #204
Remove the tmp.py file. That has just been useful for testing :)

How Has This Been Tested

Via the tests and visually!

Types of changes

New feature (non-breaking change which adds functionality)

…itat-sim into redwood-noisy-depth

… redwood-noisy-depth

dhruvbatra · 2019-09-04T20:31:01Z

A CUDA implementation that runs at ~5k FPS at 256x256 or ~200k FPS at 256x256 when using the GPU->GPU transfer from #114 !

200k?!! Damn.

Are those two images w/ and w/o noise?

Request -- is it easy to show images with varying levels of noise? (Or is that not feasible for this noise type?)

erikwijmans · 2019-09-04T20:41:35Z

Are those two images w/ and w/o noise?

Yes they are :)

Request -- is it easy to show images with varying levels of noise? (Or is that not feasible for this noise type?)

We could make that doable with this noise model. There are two different Gaussian that are sampled from and I could make the variance on those changeable.

erikwijmans · 2019-09-04T20:54:12Z

I add a simple multiplier parameter which I just multiply both the standard deviations by. Above is 2x, 3x, 4x, 5x. Seems to fairly smoothly make things worse.

… redwood-noisy-depth

erikwijmans · 2019-10-05T16:24:01Z

This is ready for review.

mathfac

That's really important step towards sim2real direction. Reviewed all the PR except details of redwood noise model implementations. Noise model registration pattern looks appealing.
Thank you for creating both CPU & GPU versions. Overall looks good to go, some minor comments.

mathfac · 2019-10-10T19:57:23Z

habitat_sim/sensors/noise_models/no_noise_model.py

+    def is_valid_sensor_type(sensor_type):
+        return True
+
+    def apply(self, x):


Do we still need this Noise Model that is producing copying? maybe we can just miss applying any noise model if it wasn't specified.

We had a copy before-hand, so I just rolled the copy into this as it felt cleaner to always apply a noise model and have a no-noise model which just returns a copy.

mathfac · 2019-10-10T19:59:37Z

habitat_sim/sensors/noise_models/redwood_depth_noise_model.py

+            else:
+                # Distort
+                undistorted_d = _undistort(
+                    int(x / xmax * 639.0 + 0.5), int(y / ymax * 479.0 + 0.5), d, model


If there is a meaning behind all the constants in this method., let's give them reasonable names. That can help to adopt noise for specific camera type/sim2real setup later.

480x640 is the resolution of the sensor used to create the distortion model originally. This logic just rescales any given observation to 480x640 so that it can query the distortion model.

I think adding a brief comment to this effect in the code would be helpful for readability

habitat_sim/sensors/noise_models/sensor_noise_model.py

mathfac · 2019-10-10T20:00:59Z

habitat_sim/sensors/noise_models/sensor_noise_model.py

+        pass
+
+    @abc.abstractmethod
+    def apply(self, sensor_observation):


Suggested change

def apply(self, sensor_observation):

def apply(self, sensor_observation) -> Observations:

I don't believe we have a generic Observations type currently defined.

How about we expose to python and flesh out Observation from the C++ side? (Bringing this up here, but it likely makes sense to create a separate issue & PR for that)

mathfac · 2019-10-10T20:03:35Z

habitat_sim/simulator.py

@@ -391,7 +398,7 @@ def get_observation(self):
                else:
                    tgt.read_frame_rgba_gpu(self._buffer.data_ptr())

-                return self._buffer.flip(0).clone()


Got it, you moved copying logic to NoNoise. Still will copying/cloning be valid for all types of sensors?

For all visual sensors, yes. This current sensor class is really for just visual sensors. When we introduce other sensors, I would imagine those will have their own separate python wrapper.

setup.py

src/esp/sensor/RedwoodNoiseModel.cpp

… redwood-noisy-depth

docs/noise_models.rst

… redwood-noisy-depth

…itat-sim into redwood-noisy-depth

msavva

Awesome to see sensor noise models coming in! 👍LGTM in general, left a few minor comments about typos and readability.

msavva · 2019-10-22T18:35:05Z

docs/noise_models.rst

+
+.. py:function:: habitat_sim.sensors.noise_models.RedwoodDepthNoiseModel.__init__
+    :param gpu_device_id: The ID of CUDA device to use (only applicable if habitat-sim was built with ``--with-cuda``)
+    :param noise_multiplier: Multipler for the Guassian random-variables.  This reduces or increase the amount of noise


typo Guassian -> Gaussian; increase -> increases

msavva · 2019-10-22T18:36:44Z

habitat_sim/registry.py

        r"""Retrieve the move_fn register under ``name``

        :param name: The name provided to `register_move_fn`
        """
        return cls._get_impl("move_fn", name)

+    @classmethod
+    def get_noise_model(cls, name: str):
+        r"""Retrieve the noise_model register under ``name``


typo: register -> registered

msavva · 2019-10-22T18:38:58Z

habitat_sim/sensors/noise_models/no_noise_model.py

+    def apply(self, x):
+        if isinstance(x, np.ndarray):
+            return x.copy()
+        else:


Do we need to check for cases where x has a type with no clone() defined? e.g. scalar-valued sensors

Added a check to see if x is a torch tensor.

msavva · 2019-10-22T18:41:31Z

habitat_sim/sensors/noise_models/redwood_depth_noise_model.py

+
+
+@numba.jit(nopython=True)
+def _undistort(x, y, z, model):


Here it would be nice to have a succinct comment describing the undistortion model being used (or at least a pointer to the original description of this in Redwood). This will help others to understand the variable names below.

msavva · 2019-10-22T18:42:07Z

habitat_sim/sensors/noise_models/redwood_depth_noise_model.py

+            else:
+                # Distort
+                undistorted_d = _undistort(
+                    int(x / xmax * 639.0 + 0.5), int(y / ymax * 479.0 + 0.5), d, model


I think adding a brief comment to this effect in the code would be helpful for readability

msavva · 2019-10-22T18:54:07Z

habitat_sim/sensors/noise_models/sensor_noise_model.py

+        pass
+
+    @abc.abstractmethod
+    def apply(self, sensor_observation):


How about we expose to python and flesh out Observation from the C++ side? (Bringing this up here, but it likely makes sense to create a separate issue & PR for that)

msavva · 2019-10-22T18:56:45Z

src/esp/sensor/RedwoodNoiseModel.cu

+const int MODEL_N_DIMS = 5;
+const int MODEL_N_COLS = 80;
+
+__device__ float undistort(const int _x,


Similar comment as above regarding a quick pointer to aid readability of the implementation.

I added the link, http://www.alexteichman.com/octo/clams/ but that doesn't really help the implementation read-ability. I just took the code from here: http://redwood-data.org/indoor/data/simdepth.py and made it not slow.

msavva · 2019-10-22T18:59:44Z

src/esp/sensor/RedwoodNoiseModel.h

+   * @brief Constructor
+   * @param model             The distortion model from
+   *                          http://redwood-data.org/indoor/data/dist-model.txt
+   *                          The 3rd dimensions is assumed to have been


typo: dimensions -> dimension

msavva · 2019-10-22T19:00:20Z

src/esp/sensor/RedwoodNoiseModel.h

+
+  /**
+   * @brief Simulates noisy depth from clean depth.  The input is assumed to be
+   * on the CPU and the output will be on the CPU


Maybe add: what happens if assumption does not hold?

Bad things happen. Segfaults happen :)

msavva · 2019-10-22T19:00:29Z

src/esp/sensor/RedwoodNoiseModel.h

+
+  /**
+   * @brief Similar to @ref simulateFromCPU() but the input and output are
+   * assumed to be on the GPU


Maybe add: what happens if assumption does not hold?

Bad things happen. Segfaults happen :)

… redwood-noisy-depth

Add depth noise model!

erikwijmans and others added 21 commits July 24, 2019 15:31

CUDA!

5bbab7a

Add tester

c461879

Seems to be working!

12a90af

Add dist-model

2bae50b

Ranme file

32c448f

Fix and add clip

0293624

Merge branch 'redwood-noisy-depth' of github.com:facebookresearch/hab…

38c6994

…itat-sim into redwood-noisy-depth

Make noise models return new data always

9aa4d5c

Remove numba

7d1ee3b

Formatters

9ed6e4f

Remove include

e39a759

Support multiple devices

49ca004

Merge branch 'redwood-noisy-depth' of github.com:facebookresearch/hab…

47f0b3e

…itat-sim into redwood-noisy-depth

Change large value behavior

c0690a1

Swap to numba implemention for CPU

8ddfc35

Merge branch 'master' of github.com:facebookresearch/habitat-sim into…

c40879a

… redwood-noisy-depth

Reviving this

a1fe2ce

Should be pretty much good to go!

d369af4

Whoops

fe71349

Less strange

0265c82

CUDA cleanup

78e0b1f

erikwijmans requested review from mosra, msavva, bigbike and dhruvbatra September 4, 2019 20:28

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

Fix sort

7da6cfb

mathfac added this to the Realistic noise models milestone Sep 5, 2019

erikwijmans mentioned this pull request Sep 11, 2019

Registry #204

Merged

erikwijmans added 10 commits September 18, 2019 12:47

Merge branch 'master' of github.com:facebookresearch/habitat-sim into…

b4a2837

… redwood-noisy-depth

Merge branch 'master' of github.com:facebookresearch/habitat-sim into…

14de723

… redwood-noisy-depth

Wat

61471ce

Dealt with that

e2e70e4

Remove unneeded lines

c0069d6

Merge branch 'master' of github.com:facebookresearch/habitat-sim into…

5e5c97b

… redwood-noisy-depth

Coming along

98672af

Better doc string

4a99950

Merge branch 'master' of github.com:facebookresearch/habitat-sim into…

4f1e737

… redwood-noisy-depth

Remove tmp.py

6d3277c

erikwijmans marked this pull request as ready for review October 4, 2019 21:40

erikwijmans requested a review from mosra October 4, 2019 21:40

erikwijmans changed the title ~~[WIP] Redwood noisy depth~~ Redwood noisy depth Oct 5, 2019

mathfac approved these changes Oct 10, 2019

View reviewed changes

erikwijmans added 2 commits October 10, 2019 16:45

Merge branch 'master' of github.com:facebookresearch/habitat-sim into…

e3b935b

… redwood-noisy-depth

Address comments

bf7ffcf

mosra reviewed Oct 11, 2019

View reviewed changes

docs/noise_models.rst Show resolved Hide resolved

erikwijmans added 3 commits October 11, 2019 09:17

Add formatting and explicit

83ac606

Merge branch 'master' of github.com:facebookresearch/habitat-sim into…

93834d3

… redwood-noisy-depth

Merge branch 'redwood-noisy-depth' of github.com:facebookresearch/hab…

7598a3c

…itat-sim into redwood-noisy-depth

msavva approved these changes Oct 22, 2019

View reviewed changes

erikwijmans added 3 commits October 22, 2019 17:57

Add comments

6e0b694

Add pass-through

0a8bc5a

Merge branch 'master' of github.com:facebookresearch/habitat-sim into…

1d31881

… redwood-noisy-depth

erikwijmans merged commit e4afe2c into master Nov 7, 2019

erikwijmans deleted the redwood-noisy-depth branch November 7, 2019 18:04

Ram81 pushed a commit to Ram81/habitat-web-sim that referenced this pull request Dec 10, 2020

Redwood noisy depth (facebookresearch#189)

1328a54

Add depth noise model!

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Redwood noisy depth #189

Redwood noisy depth #189

erikwijmans commented Sep 4, 2019 •

edited

Loading

dhruvbatra commented Sep 4, 2019

erikwijmans commented Sep 4, 2019

erikwijmans commented Sep 4, 2019

erikwijmans commented Oct 5, 2019

mathfac left a comment

mathfac Oct 10, 2019

erikwijmans Oct 10, 2019

mathfac Oct 10, 2019

erikwijmans Oct 10, 2019

msavva Oct 22, 2019

mathfac Oct 10, 2019

erikwijmans Oct 10, 2019

msavva Oct 22, 2019

mathfac Oct 10, 2019

erikwijmans Oct 10, 2019

msavva left a comment

msavva Oct 22, 2019

msavva Oct 22, 2019

msavva Oct 22, 2019

erikwijmans Oct 22, 2019

msavva Oct 22, 2019

msavva Oct 22, 2019

msavva Oct 22, 2019

msavva Oct 22, 2019

erikwijmans Oct 22, 2019

msavva Oct 22, 2019

msavva Oct 22, 2019

erikwijmans Oct 22, 2019

msavva Oct 22, 2019

erikwijmans Oct 22, 2019

	def apply(self, sensor_observation):
	def apply(self, sensor_observation) -> Observations:

Redwood noisy depth #189

Redwood noisy depth #189

Conversation

erikwijmans commented Sep 4, 2019 • edited Loading

Motivation and Context

How Has This Been Tested

Types of changes

dhruvbatra commented Sep 4, 2019

erikwijmans commented Sep 4, 2019

erikwijmans commented Sep 4, 2019

erikwijmans commented Oct 5, 2019

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

Choose a reason for hiding this comment

msavva 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

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

erikwijmans commented Sep 4, 2019 •

edited

Loading