Noise model #2
Noise model #2
Conversation
| #include <iomanip> | ||
| #include <string> | ||
| #include <map> | ||
| #include <random> |
There was a problem hiding this comment.
Include files that aren't used directly in the header itself should go in the .cpp file. This keeps your interfaces cleaner and helps with build times on large projects.
In this case all the includes but vector can go into the source file.
| #include <math.h> | ||
| #include <vector> | ||
| #include <iostream> | ||
| #include <iomanip> |
There was a problem hiding this comment.
I think string, iostream, iomanip, map are left over copy paste from an example. My gut tells me that only vector, math, and random are needed.
| #ifndef NOISE_MODEL_H | ||
| #define NOISE_MODEL_H | ||
|
|
||
| #include <math.h> |
There was a problem hiding this comment.
Prefer the C++ equivalent header <cmath> over the C include math.h.
They define the same things but the cmath includes functions in the std:: namespace.
|
|
||
| namespace gl_depth_sim | ||
| { | ||
| std::vector<float> noise(std::vector<float> distance); |
There was a problem hiding this comment.
While I know what you're trying to do here, others will not. Instead of noise, please consider a name that is more descriptive. Perhaps applyKinectNoise. I'd also like to see a brief comment (see the other headers) that describes what this does and gives a shout-out to the paper used as a guide.
|
|
||
| namespace gl_depth_sim | ||
| { | ||
| std::vector<float> noise(std::vector<float> distance); |
There was a problem hiding this comment.
Passing and returning very large objects (like these vectors) can be very expensive. Consider passing the input as a constant reference: const std::vector<float>& distance.
There's arguments to be made that returning by value is not so bad, but you do have options:
- Return with an output parameter (so that memory can be re-used)
void noise(const std::vector<float>& distance_in, std::vector<float>& distance_out);- Modify the vector "in-place":
void (std::vector<float>& distance);| const auto depth_img = sim.render(pose); | ||
| auto depth_img = sim.render(pose); | ||
|
|
||
| depth_img.data = gl_depth_sim::noise(depth_img.data); |
There was a problem hiding this comment.
Please add a ROS parameter that optionally applies the noise. This should be something that people opt into.
| std::seed_seq seed2{r(), r(), r(), r(), r(), r(), r(), r()}; | ||
| std::mt19937 e2(seed2); | ||
|
|
||
| for (int i = 0 ; i < distance.size() ;++i){ |
There was a problem hiding this comment.
Sign/unsigned comparison.
distances.size() returns an unsigned type. When using STL containers like this, using std::size_t is usually good enough. E.g. int -> std::size_t.
There was a problem hiding this comment.
Alternatively you can re-write the entire loop using C++11 range-for. For example.
for (float& r : distance)
{
// ...
r = normal_dist(e2);
}
There was a problem hiding this comment.
@asct Thanks for taking the time to prepare this. I've left you a bunch of notes. Feel free to ask for clarification.
|
|
||
| for (int i = 0 ; i < distance.size() ;++i){ | ||
| // Seed with a real random value, if available | ||
| segma= 0.0012+0.0019* pow(distance[i]-0.4,2); |
There was a problem hiding this comment.
All of these numbers, or literals, are of the double type. Not a huge deal, but given that you're starting with floats its not unreasonable to stick with the floating point math here. You can change a literal to a float by appending f, e.g. 0.0012f.
| segma= 0.0012+0.0019* pow(distance[i]-0.4,2); | ||
| float r = distance[i]; | ||
| // Generate a normal distribution around that mean | ||
| std::normal_distribution<> normal_dist(r, segma); |
There was a problem hiding this comment.
Likewise this line creates a normal distribution that is of type double. The <> brackets indicate that it is of the default template type. If you look at the docs here you'll see that means double. This could actually make difference because the random number generator will be asking for twice the random bits that it absolutely needs. Consider specifying the distribution as having template type float: std::normal_distribution<float>.
| float segma; | ||
| std::random_device r; | ||
| std::seed_seq seed2{r(), r(), r(), r(), r(), r(), r(), r()}; | ||
| std::mt19937 e2(seed2); |
There was a problem hiding this comment.
A thought:
Re-creating these every function call is probably quite expensive. You might consider moving the std::mt19937 object into a struct or something thats passed as an additional argument to the function. That would also let people control the seed sequence used to generate the noise. As it stands, they'll get a totally new one for each operation.
Don't worry about it for now, but if you don't understand the idea of random number seeds then please look it up.
ready for review