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

Dataset extraction API #483

Merged
merged 11 commits into from
Feb 26, 2020
Merged

Dataset extraction API #483

merged 11 commits into from
Feb 26, 2020

Conversation

mpiseno
Copy link

@mpiseno mpiseno commented Feb 10, 2020

Motivation and Context

Added dataset extraction API to habitat-sim that allows users to specify a scene file and get back a bunch of images that can easily be fed into a PyTorch model or saved for later use. The API supports RGBA, depth, and semantic images as well as allows the user to specify image size.

How Has This Been Tested

Created two tests in tests/test_dataset_extraction.py that:

  1. Create a topdown view
  2. input a scene file to the data extractor and feed it through a pytorch model

Types of changes

  • New feature (non-breaking change which adds functionality)

Example Output

Screen Shot 2020-02-13 at 3 18 02 PM

Screen Shot 2020-02-13 at 3 12 45 PM

Screen Shot 2020-02-13 at 3 12 54 PM

Screen Shot 2020-02-13 at 3 13 08 PM

Screen Shot 2020-02-13 at 3 13 19 PM

@mpiseno mpiseno changed the title Dataset extraction Dataset extraction API Feb 10, 2020
@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Feb 10, 2020
@@ -229,6 +231,8 @@ struct PathFinder::Impl {

std::pair<vec3f, vec3f> bounds() const { return bounds_; };

std::vector<std::vector<bool>> getTopDownView(const float res);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be an Eigen::Matrix<bool, Eigen::Dynamic, Eigen::Dynamic> matrix -- that'll get sent over to python as a numpy array with zero copy :)

@@ -1128,6 +1132,42 @@ bool PathFinder::Impl::isNavigable(const vec3f& pt,
return true;
}

std::vector<std::vector<bool>> PathFinder::Impl::getTopDownView(
const float res) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The caller should be responsible for providing a height.

@@ -229,6 +231,8 @@ struct PathFinder::Impl {

std::pair<vec3f, vec3f> bounds() const { return bounds_; };

std::vector<std::vector<bool>> getTopDownView(const float res);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's give res a more intelligible name. Seems like it is being used as metersPerPixel?

bound1, bound2 = self.pathfinder.get_bounds()
startw = min(bound1[0], bound2[0])
starth = min(bound1[2], bound2[2])
starty = self.pathfinder.get_random_navigable_point()[
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you have the caller send in the height for generating the top-down map, then you can get this exactly.


import habitat_sim
import habitat_sim.bindings as hsim
from examples.settings import make_cfg
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not be pulling something in from examples into the core of habitat-sim.

new_state.rotation = rot
self.sim.agents[0].set_state(new_state)
obs = self.sim.get_sensor_observations()
sample = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Color is output as RGB-A, so you'll want to either chop-off the alpha channel or change the name.

std::vector<bool> curRow;
for (int w = 0; w < widthResolution; w++) {
vec3f point = vec3f(curWidth, navigableHeight, curHeight);
if (isNavigable(point, 0.5)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will need to change for the Eigen matrix, but anyways, you can do curRow.push_back(isNavigable(point, 0.5)) here.

@codecov
Copy link

codecov bot commented Feb 11, 2020

Codecov Report

Merging #483 into master will increase coverage by 1.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #483      +/-   ##
==========================================
+ Coverage   59.19%   60.22%   +1.02%     
==========================================
  Files         165      168       +3     
  Lines        7340     7570     +230     
  Branches       84       84              
==========================================
+ Hits         4345     4559     +214     
- Misses       2995     3011      +16     
Flag Coverage Δ
#CPP 54.78% <100.00%> (+0.22%) ⬆️
#JavaScript 10.00% <ø> (ø) ⬆️
#Python 80.72% <87.55%> (+0.99%) ⬆️
Impacted Files Coverage Δ
tests/test_data_extraction.py 100.00% <0.00%> (ø)
habitat_sim/utils/data/data_extractor.py 84.14% <0.00%> (ø)
habitat_sim/utils/data/pose_extractor.py 86.31% <0.00%> (ø)
habitat_sim/simulator.py 95.54% <0.00%> (+1.21%) ⬆️
habitat_sim/utils/common.py 66.21% <0.00%> (+9.45%) ⬆️

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 05db83e...3ec5eaf. Read the comment docs.

@mpiseno
Copy link
Author

mpiseno commented Feb 11, 2020

I had to force-push to fix some difficult errors that caused api tests to fail. Will address the comments above @erikwijmans thanks for the feedback.

requirements.txt Outdated
@@ -5,3 +5,5 @@ numpy-quaternion
pillow
scipy>=1.3.0
tqdm
torch
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we've avoided adding torch as a dependency so far. @erikwijmans -- thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we should continue to avoid it as a hard dependency. @mpiseno we can have the dataset extractor class be API-compatible and then mix-in a torch dataset in the training examples or something. It could also be a soft dependency (i.e. you will only need it if you want to use this code).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only think I actually use pytorch for is in the tests in tests/test_dataset_extraction. I could remove this without any changes to the ImageExtractor class, but I think @mathfac wanted to run the data through a pytorch model in testing?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fine, we have a soft dependency on pytorch for some tests already (--with-cuda relies on torch in places), but that soft dependency doesn't need to be in requirements.txt

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it sufficient to simply remove torch from requirements.txt? Will the soft dependency during testing be handled appropriately even if i import torch in my tests/test_dataset_extraction.py file?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, just removing it from requirements.txt is sufficient. All of a "soft" dependency is in these terms is something you don't list in requirements.txt and the user will only need to install if they actually want to run certain code (as opposed to just importing the function).

Michael Piseno added 2 commits February 11, 2020 17:50
@mpiseno
Copy link
Author

mpiseno commented Feb 12, 2020

Fixed everything you gave feedback for @erikwijmans thank you for the help!

Copy link
Contributor

@erikwijmans erikwijmans left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One layout request: Can you break ImageExtractor and PoseExtractor into their own files? Both seem like they provide enough functionality to warrant being in their own file and it also makes quickly remembering where they are easier :)

return (startw, starty, starth) # width, y, height


class TopdownView(object):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this class do? Seems like it's just a helper function?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now it is essentially a helper function. @msbaines thought the topdown functionality could be useful in other contexts in the future, which is why I made it separate.


class TopdownView(object):
def __init__(self, sim, height, pixels_per_meter=0.1):
self.topdown_view = np.array(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should no longer need the np.array here.

"silent": True,
}

return make_cfg(sim_settings)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the dictionary and helper function doesn't really seem necessary here. Just making the config in this method would be cleaner IMO.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the next PR im working on that depends on this one, I need to call this method from multiple classes, but for this branch it does seem cleaner to do what you suggested.


def _compute_quat(self, cam_normal):
"""Rotations start from -z axis"""
return quat_from_two_vectors(np.array([0, 0, -1]), cam_normal)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return quat_from_two_vectors(np.array([0, 0, -1]), cam_normal)
return quat_from_two_vectors(habitat_sim.geo.FRONT, cam_normal)

def close(self):
r"""Deletes the instance of the simulator. Necessary for instatiating a different ImageExtractor.
"""
self.sim.close()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add an self.sim = None bellow and then guard all this with if self.sim is not None: that way you can call close twice.

Copy link
Contributor

@erikwijmans erikwijmans left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple comments

@@ -1222,5 +1258,17 @@ std::pair<vec3f, vec3f> PathFinder::bounds() const {
return pimpl_->bounds();
}

// std::vector<std::vector<bool>> PathFinder::getTopDownView(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove.

@@ -0,0 +1,192 @@
import collections
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

snake_case filename to match the rest of the codebase.

@@ -0,0 +1,183 @@
import math
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same request for filename.

@@ -0,0 +1,234 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We banished notebooks a while ago as they don't play nice with git, CC @abhiskk

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had asked Michael to make one so that it serves as a tutorial. What would be better if not a notebook?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We banished them from git, they still exist, just not in git.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oleksandr said I should make a tutorial on the habitat website that reflects the same information

@mpiseno
Copy link
Author

mpiseno commented Feb 26, 2020

I believe @mathfac said the 1 test that is currently failing is a known issue and if everything else is passing then this PR should be good. If that's the case, I'm just waiting on PR approval to merge.

@mpiseno mpiseno merged commit e083724 into master Feb 26, 2020
@mpiseno mpiseno deleted the dataset-extraction branch February 26, 2020 23:28
Ram81 pushed a commit to Ram81/habitat-web-sim that referenced this pull request Dec 10, 2020
* created data extraction API for habitat-sim

* height passed in as arg to getTopdownView and removed examples/settings import

* added jupyter notebook example for Dataset Extraction API and changed getTopdownView to return Eigen Matrix

* formatting

* moved PoseExtractor to separate file and other formatting changes

* removed torch dependency

* failing api tests fix

* this commit builds

* modified close method in ImageExtractor

* removed jupyter notebook and modified file names to match other python files format

* small test change
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Do not delete this pull request or issue due to inactivity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants