Skip to main content
deleted 47 characters in body
Source Link
Cris Luengo
  • 7k
  • 1
  • 15
  • 37

This is a neat class with a clear interface.

I would consider adding a constructor that takes an image as input, and computes the full histogram for the image.

The only issue I see with this class is that it always uses a map, even for the example uint8 image where a vector would be much more efficient (and which you’ve used in your previous posts on histogram computation). One solution is to have both a vector and a map as class members, and a Boolean that says which of the two is being used. For some histograms (eg the color image in a previous post of yours) the data is stored in a map, for others (uint8 and uint16) in a vector. This does add a test for each access, but that might be OK, and still faster than a lookup in a map. Another solution would involvecould be to have a base class for the interface, and two derived classessingle histogram member, one for eachbut have its type of histogrambe selected at compile time (see G. I personally don’t like class hierarchies, but it’s a valid approachSliepen’s comment below).

In this bit of code:

            if (histogram.contains(input))
            {
                return histogram.at(input);
            }

you are doing the lookup twice. Instead, use std::map::find() to get an iterator to the element. If you don’t get the end iterator back, you can dereference it read the value.

This bit of code is also redundant:

            if (histogram.contains(input))
            {
                ++histogram[input];
            }
            else
            {
                histogram.emplace(input, std::size_t{ 1 });
            }

If input is not in the map, then histogram[input] creates that item with a default-initialized size_t, which is zero. So you should be able to just do ++histogram[input] in either case. You are making us of this already in the operator+ case!

This is a neat class with a clear interface.

I would consider adding a constructor that takes an image as input, and computes the full histogram for the image.

The only issue I see with this class is that it always uses a map, even for the example uint8 image where a vector would be much more efficient (and which you’ve used in your previous posts on histogram computation). One solution is to have both a vector and a map as class members, and a Boolean that says which of the two is being used. For some histograms (eg the color image in a previous post of yours) the data is stored in a map, for others (uint8 and uint16) in a vector. This does add a test for each access, but that might be OK, and still faster than a lookup in a map. Another solution would involve a base class for the interface, and two derived classes, one for each type of histogram. I personally don’t like class hierarchies, but it’s a valid approach.

In this bit of code:

            if (histogram.contains(input))
            {
                return histogram.at(input);
            }

you are doing the lookup twice. Instead, use std::map::find() to get an iterator to the element. If you don’t get the end iterator back, you can dereference it read the value.

This bit of code is also redundant:

            if (histogram.contains(input))
            {
                ++histogram[input];
            }
            else
            {
                histogram.emplace(input, std::size_t{ 1 });
            }

If input is not in the map, then histogram[input] creates that item with a default-initialized size_t, which is zero. So you should be able to just do ++histogram[input] in either case. You are making us of this already in the operator+ case!

This is a neat class with a clear interface.

I would consider adding a constructor that takes an image as input, and computes the full histogram for the image.

The only issue I see with this class is that it always uses a map, even for the example uint8 image where a vector would be much more efficient (and which you’ve used in your previous posts on histogram computation). One solution is to have both a vector and a map as class members, and a Boolean that says which of the two is being used. For some histograms (eg the color image in a previous post of yours) the data is stored in a map, for others (uint8 and uint16) in a vector. This does add a test for each access, but that might be OK, and still faster than a lookup in a map. Another solution could be to have a single histogram member, but have its type be selected at compile time (see G. Sliepen’s comment below).

In this bit of code:

            if (histogram.contains(input))
            {
                return histogram.at(input);
            }

you are doing the lookup twice. Instead, use std::map::find() to get an iterator to the element. If you don’t get the end iterator back, you can dereference it read the value.

This bit of code is also redundant:

            if (histogram.contains(input))
            {
                ++histogram[input];
            }
            else
            {
                histogram.emplace(input, std::size_t{ 1 });
            }

If input is not in the map, then histogram[input] creates that item with a default-initialized size_t, which is zero. So you should be able to just do ++histogram[input] in either case. You are making us of this already in the operator+ case!

Source Link
Cris Luengo
  • 7k
  • 1
  • 15
  • 37

This is a neat class with a clear interface.

I would consider adding a constructor that takes an image as input, and computes the full histogram for the image.

The only issue I see with this class is that it always uses a map, even for the example uint8 image where a vector would be much more efficient (and which you’ve used in your previous posts on histogram computation). One solution is to have both a vector and a map as class members, and a Boolean that says which of the two is being used. For some histograms (eg the color image in a previous post of yours) the data is stored in a map, for others (uint8 and uint16) in a vector. This does add a test for each access, but that might be OK, and still faster than a lookup in a map. Another solution would involve a base class for the interface, and two derived classes, one for each type of histogram. I personally don’t like class hierarchies, but it’s a valid approach.

In this bit of code:

            if (histogram.contains(input))
            {
                return histogram.at(input);
            }

you are doing the lookup twice. Instead, use std::map::find() to get an iterator to the element. If you don’t get the end iterator back, you can dereference it read the value.

This bit of code is also redundant:

            if (histogram.contains(input))
            {
                ++histogram[input];
            }
            else
            {
                histogram.emplace(input, std::size_t{ 1 });
            }

If input is not in the map, then histogram[input] creates that item with a default-initialized size_t, which is zero. So you should be able to just do ++histogram[input] in either case. You are making us of this already in the operator+ case!