-3

I am a beginner in C++ and I'm writing a function that is supposed to find the closest point in a set of points ([x1,...xn]=positions_x,[y1,...yn]=positions_y) to a given point (x0,y0).

Here is my function:

double nearestPoint(double x, double y)
{
    double ind_min = 0;
    double xi = 0;
    double yi = 0;
    double dist_mini = 1000000;
    int i;
    for (i = 0; i < 50000; i=i+1)
        xi = positions_x[i];
        yi = positions_y[i];
        double a = pow(x-xi,2);
        double b = pow(y-yi,2);
            if (sqrt(a+b)<=dist_mini)
                dist_mini = sqrt(a+b);
                ind_min = i;
    return ind_min;
}

However, this always returns the exact same point. After investigation, it seems that the value of the "dist_mini" variable remains unchanged even if sqrt(a+b) is smaller. As a consequence, ind_min remains unchanged.

If you have any ideas it would really helpful,

Thank you

Sven van den Boogaart
  • 11,833
  • 21
  • 86
  • 169
Baptiste
  • 7
  • 3
  • 1
    What is `positions_x` and `positions_y`? – tadman Jun 27 '23 at 14:39
  • 8
    Indentation is arbitrary. Add `{` `}` to execute mutliple things in a loop. – KamilCuk Jun 27 '23 at 14:40
  • 6
    Your for loop is only `for (i = 0; i < 50000; i=i+1) xi = positions_x[i];`. Everything else after that is not part of the loop. – NathanOliver Jun 27 '23 at 14:41
  • Might be time to pick up a good [reference book](https://stackoverflow.com/questions/388242/the-definitive-c-book-guide-and-list) to help guide your learning. – tadman Jun 27 '23 at 14:41
  • Same problem with your `if`, for that matter. – Nathan Pierson Jun 27 '23 at 14:41
  • C++ does not care about indentation, unlike python - but please keep the clear indentation style. Even though it doesn't do anything for the compiler, it sure makes the code nice to read for humans. – Ted Lyngmo Jun 27 '23 at 14:47
  • Pretty unlucky design, by the way – the coordinates of points are really closely related and you shouldn't spread these all around by placing them into separate arrays. Far better is having your own `struct Point { double x; double y; }; ` and have an array or vector of these. Additional benefit: you can then easily iterate over in a range-based for loop: `for(auto& p : points) { auto diffx = x - p.x; auto diffy = x - p.y; /*...*/ }` – Aconcagua Jun 27 '23 at 14:51
  • And don't use `pow` for squaring, that's just overkill, simply do `double squareX = x*x`. – Aconcagua Jun 27 '23 at 14:53
  • 1
    You also don't need to take the square root of the squared distances. If a squared distance is closer than another squared distance, so will the square root of that squared distance. – Ted Lyngmo Jun 27 '23 at 14:55
  • 1
    Also note that since `sqrt` is monotonic you don't actually need to take the `sqrt` of anything. For positive numbers `a` and `b`, `a < b` if and only if `sqrt(a) < sqrt(b)`, so even if what you actually care about is the latter comparison you can get away with only doing the former, which is computationally cheaper. – Nathan Pierson Jun 27 '23 at 14:56
  • Your initial value for the minimum is potentially problematic as well, unless you guarantee smaller distances by whatever means. I personally would simply go with `std::numeric_limits::infinity()`, and be on the safe side. – Aconcagua Jun 27 '23 at 14:56
  • And you *absolutely* shouldn't return *indices* as `double`! Correct type for actually is `size_t` (not `int` either) – you need to include `` for. Finally get used to limit the scope of variables as restrictively as possible. You don't need `i` *after* the loop, so have `for(int /* or possibly better size_t*/ i = 0; /*...*/)`. – Aconcagua Jun 27 '23 at 15:00
  • 1
    @Aconcagua Similarly `xi` and `yi` aren't needed outside the loop either – Nathan Pierson Jun 27 '23 at 15:01
  • What makes you that sure that you always have 50000 points? If that's a `std::vector`, only iterate up to that one's size, if you have a static array trace how many points actually are in use and iterate up to there. If *all* points in an array are always in use anyway then still avoid magic numbers, you are better of with `std::array::size` if you use such one (recommendable) or with `sizeof(array)/sizeof(*array)` if you use raw arrays. – Aconcagua Jun 27 '23 at 15:04
  • *"even if sqrt(a+b) is smaller"* -- You should show this! Drop all the excess code and focus on the problem. Initialize `a` and `b` to values that demonstrate the problem. (Do not try to demonstrate "find the closest point".) Keep your **example** code simple, like `int main() { double dist_mini = 1000000; double a = 1; double b = 0; if (sqrt(a+b)<=dist_mini) return 1; else return 0; }` Get rid of unnecessary code so that the bug has no place to hide! – JaMiT Jun 27 '23 at 15:05
  • @JaMiT That doesn't work here, the bug is the missing braces, your variant hides that away entirely... – Aconcagua Jun 27 '23 at 15:08
  • An improved variant considering the preceding comments might look like [this](https://godbolt.org/z/PKdbbrbnM) – note that additionally providing the points as parameter improves re-usability for not relying on a global any more (you could look up with one and the same function the nearest point different point vectors...). – Aconcagua Jun 27 '23 at 15:22
  • @Aconcagua *"That doesn't work here"* -- To the contrary, it (meaning "keep it simple") works wonderfully here. It's just that the example I gave is a starting point rather than a destination. (Correct, my example of example code does not demonstrate the problem. Comments don't give me enough room for a full demonstration. The remaining steps -- adding back pieces -- were left to the reader.) – JaMiT Jun 27 '23 at 23:26

1 Answers1

5

Add curly brackets ({ & }) to ensure all statements are executed in the for- and if-block. Without the curly brackets, only the first statement will be inside the block.

for (i = 0; i < 50000; i=i+1)
{
    xi = positions_x[i];
    yi = positions_y[i];
    double a = pow(x-xi,2);
    double b = pow(y-yi,2);
    if (sqrt(a+b)<=dist_mini)
    {
        dist_mini = sqrt(a+b);
        ind_min = i;
    }
}
return ind_min;
Aziz
  • 20,065
  • 8
  • 63
  • 69
  • Thanks a lot to you and to others who answered my question. It was indeed a problem with the brackets! – Baptiste Jun 27 '23 at 14:54