-
Notifications
You must be signed in to change notification settings - Fork 419
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
Conversation
src/esp/nav/PathFinder.cpp
Outdated
@@ -229,6 +231,8 @@ struct PathFinder::Impl { | |||
|
|||
std::pair<vec3f, vec3f> bounds() const { return bounds_; }; | |||
|
|||
std::vector<std::vector<bool>> getTopDownView(const float res); |
There was a problem hiding this comment.
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 :)
src/esp/nav/PathFinder.cpp
Outdated
@@ -1128,6 +1132,42 @@ bool PathFinder::Impl::isNavigable(const vec3f& pt, | |||
return true; | |||
} | |||
|
|||
std::vector<std::vector<bool>> PathFinder::Impl::getTopDownView( | |||
const float res) { |
There was a problem hiding this comment.
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.
src/esp/nav/PathFinder.cpp
Outdated
@@ -229,6 +231,8 @@ struct PathFinder::Impl { | |||
|
|||
std::pair<vec3f, vec3f> bounds() const { return bounds_; }; | |||
|
|||
std::vector<std::vector<bool>> getTopDownView(const float res); |
There was a problem hiding this comment.
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()[ |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 = { |
There was a problem hiding this comment.
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.
src/esp/nav/PathFinder.cpp
Outdated
std::vector<bool> curRow; | ||
for (int w = 0; w < widthResolution; w++) { | ||
vec3f point = vec3f(curWidth, navigableHeight, curHeight); | ||
if (isNavigable(point, 0.5)) { |
There was a problem hiding this comment.
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.
2b007b1
to
17131ca
Compare
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
… getTopdownView to return Eigen Matrix
Fixed everything you gave feedback for @erikwijmans thank you for the help! |
There was a problem hiding this 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): |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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( |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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() |
There was a problem hiding this comment.
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.
625beb0
to
1b7ae03
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple comments
src/esp/nav/PathFinder.cpp
Outdated
@@ -1222,5 +1258,17 @@ std::pair<vec3f, vec3f> PathFinder::bounds() const { | |||
return pimpl_->bounds(); | |||
} | |||
|
|||
// std::vector<std::vector<bool>> PathFinder::getTopDownView( |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 @@ | |||
{ |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
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. |
* 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
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:
Types of changes
Example Output