Register a SA Forums Account here!
JOINING THE SA FORUMS WILL REMOVE THIS BIG AD, THE ANNOYING UNDERLINED ADS, AND STUPID INTERSTITIAL ADS!!!

You can: log in, read the tech support FAQ, or request your lost password. This dumb message (and those ads) will appear on every screen until you register! Get rid of this crap by registering your own SA Forums Account and joining roughly 150,000 Goons, for the one-time price of $9.95! We charge money because it costs us money per month for bills, and since we don't believe in showing ads to our users, we try to make the money back through forum registrations.
 
  • Post
  • Reply
rjmccall
Sep 7, 2007

no worries friend
Fun Shoe

Spiritus Nox posted:

I really appreciate you going so in depth, by the by. :)

Just sitting here not getting the work I'm supposed to be doing done. :)

Spiritus Nox posted:

I think I'm following you on this - so I'd basically be passing Edge structures onto the priority queue this way, right? And I can pull that edge off the queue, compare its weight to dist[whatever that Edge is pointing to], and go from there? Just want to make sure I understand you before I dick around with this algorithm some more.

Yeah, you can think of the path as an edge all the way to the destination. As a stylistic point, in "real" code you would want to (at least) use a typedef and a comment saying "I just need a pair of a node and a weight and it turns out that Edge is exactly that." Otherwise, it'd be pretty easy to get confused reading the code and think that for some reason you were queuing up edges from the actual graph.

Adbot
ADBOT LOVES YOU

Spiritus Nox
Sep 2, 2011

rjmccall posted:

Just sitting here not getting the work I'm supposed to be doing done. :)


Yeah, you can think of the path as an edge all the way to the destination. As a stylistic point, in "real" code you would want to (at least) use a typedef and a comment saying "I just need a pair of a node and a weight and it turns out that Edge is exactly that." Otherwise, it'd be pretty easy to get confused reading the code and think that for some reason you were queuing up edges from the actual graph.

Excellent! I'm sick of staring at code and I've got other things that need doing right now, but this gives me something to go on that isn't 'write an entire new class you've never written before.' Thanks a bunch!

Spiritus Nox
Sep 2, 2011

Here's what I've got so far - this currently throws wrong answers - I think it's only taking the first weight it finds for each node and refusing to overwrite it, but I'm not entirely sure. It also throws a run-time error (SIGABRT) on the upload site.

code:
//Edge Comparitor
class CompareEdge
{
  public:
    bool operator()(Edge e1, Edge e2)
    {
      bool returnMe = false;
      if(e1.weight < e2.weight)
      {
        returnMe = true;
      }
      return returnMe;
    }
};

//Dijkstra:

vector<int> Graph::shortestPath1(int s)
{

  priority_queue<Edge, vector<Edge>, CompareEdge> Q;
  vector<int> dist(this->numNodes, INFINITY);
  Edge e;

  dist[s] = 0;

  for(int i = 0; i < this->numNodes; i++)
  {

    e.from = i;
    for(int k = 0; k < this->numNodes; k++)
    {
      e.to = k;
      if(i == k)
      {
        e.weight = 0;
      }
      else
      {
        e.weight = INFINITY;
      }
      Q.push(e);
    }

  }

  while(!Q.empty())
  {
    e = Q.top();
    Q.pop();
    if(dist[e.to] == e.weight)
    {
      for(int i = 0; i < Nodes[e.from].numAdjacent; i++)
      {
        if(dist[e.from] + getWeight(e.from, Nodes[e.from].adjacentTo[i]) < dist[Nodes[e.from].adjacentTo[i]])
        {
          e.to = Nodes[e.from].adjacentTo[i];
          dist[e.to] = dist[e.from] + getWeight(e.from, e.to);
          e.weight = dist[e.to];
          Q.push(e);
        }
      }
    }
  }

  return dist;
}


Any thoughts?

Spiritus Nox fucked around with this message at 19:07 on Dec 5, 2012

rjmccall
Sep 7, 2007

no worries friend
Fun Shoe

Spiritus Nox posted:

code:
  for(int i = 0; i < this->numNodes; i++)
  {

    e.from = i;
    for(int k = 0; k < this->numNodes; k++)
    {
      e.to = k;
      if(i == k)
      {
        e.weight = 0;
      }
      else
      {
        e.weight = INFINITY;
      }
      Q.push(e);
    }

  }

What is this loop? This is not in the algorithm.

Spiritus Nox posted:

code:
      for(int i = 0; i < Nodes[e.from].numAdjacent; i++)

Okay, there's a problem here. You're not really enqueuing an edge; you're enqueuing a node. But you've really confused yourself into thinking of it as a path through the graph, as if both e.from and e.to were meaningful.

(It's not unreasonable to think of what you're enqueuing as a path, with e.from being the first node in the path, e.to being the last, and e.weight being the total weight of the path. However, recall that Dijkstra's computes the lengths of paths starting at the source node. If you were consistent about this, logically e.from should always be s — which is useless to store separately. That's why you're just storing nodes.)

This is exactly why it's usually a bad idea to reuse classes for different purposes like this: it's really easy to get fooled into thinking that the class's entire interface is meaningful. (There are other reasons, too, but this is a big one.) Remember that all you wanted was a pair of a node and a distance. You decided to reuse the Edge class, and that class actually has *two* references to nodes in it, from and to. Okay, you can still make that work — just use one of them, right? Except now you need to remember which one you're using, and it's easy to get that wrong.

Making a new class is not difficult. If it's just for your private use in one file, there's no need to pretty it up, and you don't need to put it in a header or anything. Just do something like this:

code:
namespace {
  struct NodeWithDistance {
    Node *node;
    int distance;
  };
}
(The namespace thing just tells the compiler that this struct is only being used in this file. C and C++ are so amazingly primitive that the compiler cannot actually figure this out just from the fact that you wrote it in a .cpp file instead of a header. It's not absolutely necessary, but it's good style, and in a more complicated case it can matter — for example, if you tried to make different structs with the same name in multiple files.)

Spiritus Nox
Sep 2, 2011

Right, duh. We only need one node because the only distance that matters is the total distance from the source to that node. :doh: The loop you asked about was just me initializing a bunch of Edges to pass into the Queue, obviously that was part of my misconception. I'll work on that.

nielsm
Jun 1, 2009



Spiritus Nox posted:

C++ code:
//Edge Comparitor
class CompareEdge
{
  public:
    bool operator()(Edge e1, Edge e2)
    {
      bool returnMe = false;
      if(e1.weight < e2.weight)
      {
        returnMe = true;
      }
      return returnMe;
    }
};
Any thoughts?

Mostly stylistic, but consider rewriting this as:
C++ code:
struct CompareEdge {
    bool operator()(Edge const &e1, Edge const &e2) {
        return e1.weight < e2.weight;
    }
};
First, struct is essentially the same as class, only a struct has default visibility public instead of private. It doesn't really matter which you use in this case, but I prefer struct for things like this.

Second, when you pass non-trivial objects into a function, you should usually pass them as const references unless you have a good reason not to. It can allow the compiler to make better optimisations. When you pass by value as in your current code, you are actually telling the compiler you want to get a complete copy of each object passed to the function, so if your class has a lot of data or is complex to copy, that can potentially be a performance hog.

Last, when you are doing a simple comparison and returning a boolean, remember that the result of the comparison operators is boolean. You don't need a temporary result variable for this. Sometimes having a result variable can make code easier to read, but for basic cases like this it just clutters things up.

mobby_6kl
Aug 9, 2009

by Fluffdaddy
What the poo poo is this?



Those are the same folders... Has anyone ran into this issue with VS2012? This starts happening without any changes being made to the named file. Like I'd just set a different breakpoint and suddenly debugging stops working. Reopening the file, solution or VS does nothing, though renaming the file helps for a while :confused:

Spiritus Nox
Sep 2, 2011

Not entirely sure what's wrong with this:

code:
vector<int> Graph::shortestPath1(int s)
{

  priority_queue<Path, vector<Path>, ComparePath> Q;
  vector<int> dist(this->numNodes, INFINITY);
  Path p;

  p.n = &this->Nodes[s];
  p.distance = 0;
  Q.push(p);

  dist[s] = 0;

  while(!Q.empty())
  {
    p = Q.top();
    Q.pop();
    if(dist[p.n->index] == p.distance)
    {
      for(int i = 0; i < p.n->numAdjacent; i++)
      {
        if(dist[p.n->index] + getWeight(p.n->index, p.n->adjacentTo[i]) < dist[p.n->adjacentTo[i]])
        {
          dist[p.n->adjacentTo[i]] = dist[p.n->index] + getWeight(p.n->index, p.n->adjacentTo[i]);
          p.n = &this->Nodes[p.n->adjacentTo[i]];
          p.distance = dist[p.n->adjacentTo[i]];
          Q.push(p);
        }
      }
    }
  }

  return dist;
}


//Path and Node structs:

struct Node
{
  int index;
  int weight;
  int numAdjacent;
  vector<int> adjacentTo;
  vector<Edge> adjacentEdge;
};

struct Path
{
  Node *n;
  int distance;
};


The output calculates the right path to some nodes, but classifies others as unreachable and I'm not sure what the rhyme or reason to it is - it doesn't seem to have anything to do with how many nodes out from the source any specific node is.

EDIT: Nevermind - it's because I was changing p before the loop was done with it, duh. :doh:

Spiritus Nox fucked around with this message at 21:53 on Dec 5, 2012

rjmccall
Sep 7, 2007

no worries friend
Fun Shoe
You're modifying p.n in the loop.

Spiritus Nox
Sep 2, 2011

rjmccall posted:

You're modifying p.n in the loop.

So I was. Fixed it...and now it's running too slow yet again. :negative: I think the bottleneck is my getWeight function. Here's what I've got.

code:
//Dijkstra:
vector<int> Graph::shortestPath1(int s)
{

  priority_queue<Path, vector<Path>, ComparePath> Q;
  vector<int> dist(this->numNodes, INFINITY);
  Path p, q;
  int weight;

  p.n = &this->Nodes[s];
  p.distance = 0;
  Q.push(p);

  dist[s] = 0;

  while(!Q.empty())
  {
    p = Q.top();
    Q.pop();
    if(dist[p.n->index] == p.distance)
    {
      for(int i = 0; i < p.n->numAdjacent; i++)
      {
        weight = getWeight(p.n->index, p.n->adjacentTo[i]);
        if(dist[p.n->index] + weight < dist[p.n->adjacentTo[i]])
        {
          dist[p.n->adjacentTo[i]] = dist[p.n->index] + weight;
          q.n = &this->Nodes[p.n->adjacentTo[i]];
          q.distance = dist[p.n->adjacentTo[i]];
          Q.push(q);
        }
      }
    }
  }

  return dist;
}

//GetWeight
int Graph::getWeight(int index1,int index2)
{
  int x = INFINITY;
  for(int i = 0; i < Nodes[index1].numAdjacent; i++)
  {
    if(Nodes[index1].adjacentEdge[i].to == index2)
    {
      x = Nodes[index1].adjacentEdge[i].weight;
    }
  }
  return x;
}
This is the fastest way I could figure out to get the weight earlier. If you've got a faster one, it'd be much appreciated.

rjmccall
Sep 7, 2007

no worries friend
Fun Shoe
1. You need to iterate over all the out-edges of a node.
2. For each out-edge, you actually need the edge structure, not just the destination, because you need to get the weight.
3. Your nodes already have a list of all the out-edges from the node.
4. So why are you iterating over the separate list of all the nodes adjacent to the node?
5. In fact, why do you even *have* a separate list of all the nodes adjacent to the node?

csammis
Aug 26, 2003

Mental Institution
Instead of completing the for loop you could just return the weight when you find it. A graph should not have more than one edge (and thus not more than one weight) directly connecting the node at index1 and the node at index2. If you've got verticies with a lot of connecting edges that might be slowing down the weight lookup. It's still worst-case O(n) - if this is truly where your performance degradation is then you may want to consider using a data structure with faster lookup speeds to hold a Node's Edges.

e: or what rjmccall said :)

Spiritus Nox
Sep 2, 2011

rjmccall posted:

5. In fact, why do you even *have* a separate list of all the nodes adjacent to the node?

Garbage from earlier. I've been working way too long on this stupid loving program.

Is this something like what you had in mind?

code:

int Graph::getWeight(int index1,int index2)
{
  int x = INFINITY;
  for(int i = 0; i < Nodes[index1].numAdjacent; i++)
  {
    if(Nodes[index1].adjacentEdge[i].to == index2)
    {
      x = Nodes[index1].adjacentEdge[i].weight;
      break;
    }
  }
  return x;
}

This is still too slow, by the by.

Jabor
Jul 16, 2010

#1 Loser at SpaceChem
You don't need that getWeight function at all - just read out the weight from the relevant edge when you determine that two nodes are adjacent.

Spiritus Nox
Sep 2, 2011

Jabor posted:

You don't need that getWeight function at all - just read out the weight from the relevant edge when you determine that two nodes are adjacent.

Good advice - it does indeed work without it.

It's still too slow I am :qq:ing so hard right now guys AAAAAAAGGH.

Frustration aside, here's what I've got.

code:
vector<int> Graph::shortestPath1(int s)
{

  priority_queue<Path, vector<Path>, ComparePath> Q;
  vector<int> dist(this->numNodes, INFINITY);
  Path p, q;
  int weight;

  p.n = &this->Nodes[s];
  p.distance = 0;
  Q.push(p);

  dist[s] = 0;

  while(!Q.empty())
  {
    p = Q.top();
    Q.pop();
    if(dist[p.n->index] == p.distance)
    {
      for(int i = 0; i < p.n->numAdjacent; i++)
      {
        weight = p.n->adjacentEdge[i];
        if(dist[p.n->index] + weight < dist[p.n->adjacentTo[i]])
        {
          dist[p.n->adjacentTo[i]] = dist[p.n->index] + weight;
          q.n = &this->Nodes[p.n->adjacentTo[i]];
          q.distance = dist[p.n->adjacentTo[i]];
          Q.push(q);
        }
      }
    }
  }

  return dist;
}

Jabor
Jul 16, 2010

#1 Loser at SpaceChem
Why are you using a bunch of parallel arrays when you have a perfectly good Edge class for that information?

Spiritus Nox
Sep 2, 2011

Jabor posted:

Why are you using a bunch of parallel arrays when you have a perfectly good Edge class for that information?

...I don't follow. I apologize if this is something really obvious - I'm a third semester student who's done nothing but try and get this thing working for the past 4 days now and my eyes are starting to glaze over whenever I look at this code.

Edit: Is this what you meant? (Still too slow)

code:
vector<int> Graph::shortestPath1(int s)
{

  priority_queue<Path, vector<Path>, ComparePath> Q;
  vector<int> dist(this->numNodes, INFINITY);
  Path p, q;
  int weight;

  p.n = &this->Nodes[s];
  p.distance = 0;
  Q.push(p);

  dist[s] = 0;

  while(!Q.empty())
  {
    p = Q.top();
    Q.pop();
    if(dist[p.n->index] == p.distance)
    {
      for(int i = 0; i < p.n->numAdjacent; i++)
      {
        weight = p.n->adjacentEdge[i].weight;
        if(dist[p.n->index] + weight < dist[p.n->adjacentTo[i]])
        {
          dist[p.n->adjacentEdge[i].to] = dist[p.n->index] + weight;
          q.n = &this->Nodes[p.n->adjacentEdge[i].to];
          q.distance = dist[p.n->adjacentEdge[i].to];
          Q.push(q);
        }
      }
    }
  }

  return dist;
}

Spiritus Nox fucked around with this message at 00:16 on Dec 6, 2012

chglcu
May 17, 2007

I'm so bored with the USA.

mobby_6kl posted:

What the poo poo is this?



Those are the same folders... Has anyone ran into this issue with VS2012? This starts happening without any changes being made to the named file. Like I'd just set a different breakpoint and suddenly debugging stops working. Reopening the file, solution or VS does nothing, though renaming the file helps for a while :confused:

Do you maybe have a virus scanner set to scan files on access? Just a wild guess, and I haven't seen this particular problem, but in the past I've had Visual Studio have weird interactions with virus scanners. Just a possibility to check out.

Jabor
Jul 16, 2010

#1 Loser at SpaceChem
Yes, that's what I mean.

A couple of quick questions:

1. Are you using an optimized build when checking the speed of this?
2. Are you sure you're not doing anything really slow in the rest of your code?

Spiritus Nox
Sep 2, 2011

Jabor posted:

Yes, that's what I mean.

A couple of quick questions:

1. Are you using an optimized build when checking the speed of this?
2. Are you sure you're not doing anything really slow in the rest of your code?

I'm not the one testing the speed - I turn it in on the upload site (yes, this is for school, no, I'm not cheating unless you lot start giving me code to copy), it feeds it some test data using a driver I wrote (and I don't get to see this test data), and tells me how it goes - if it was good, or too slow, or wrong answer, or segfault or what have you. My latest version gets a 'time limit exceeded' message. I believe the time limit for this particular problem is 5 seconds - which isn't especially meaningful since I have no idea what sort of graph the test site is plugging in.

I'm almost positive it's the Dijkstra's function that's wrong somehow. It's not calling any functions (apart from the Priority Queue stuff, but that's STL and my classmates have passed using it), and when I set the dist vector in my driver to all zeroes instead of calling the function the program runs in time. Something's still wrong with it.

Spiritus Nox fucked around with this message at 01:34 on Dec 6, 2012

Jabor
Jul 16, 2010

#1 Loser at SpaceChem

Spiritus Nox posted:

I'm not the one testing the speed - I turn it in on the upload site (yes, this is for school, no, I'm not cheating unless you lot start giving me code to copy), it feeds it some test data using a driver I wrote (and I don't get to see this test data), and tells me how it goes - if it was good, or too slow, or wrong answer, or segfault or what have you. My latest version gets a 'time limit exceeded' message. I believe the time limit for this particular problem is 5 seconds - which isn't especially meaningful since I have no idea what sort of graph the test site is plugging in.

I'm almost positive it's the Dijkstra's function that's wrong somehow. It's not calling any functions (apart from the Priority Queue stuff, but that's STL and my classmates have passed using it), and when I set the dist vector in my driver to all zeroes instead of calling the function the program runs in time. Something's still wrong with it.

Is the execution time of your driver included in the allowed 5 seconds?

What's the problem you're required to solve? Take one point and compute the distance to every other point?

Spiritus Nox
Sep 2, 2011

Jabor posted:

Is the execution time of your driver included in the allowed 5 seconds?

What's the problem you're required to solve? Take one point and compute the distance to every other point?

Yes and yes. The driver takes the data to make the graph (number of nodes, of edges, where to put the edges), and a starting point. Then the user enters x number of indexes (and we're guaranteed to be asked about a valid index) and expects to return the shortest path from their starting point to the node at point x. The driver itself takes about half a second to run when it just populates dist with zeroes. Here's my driver:

code:
#include <iostream>
#include "Graph.h"
 
using namespace std;
 
int main()
{
    vector<int> dist;
    int nodes;
    int edges;
    int queries;
    int start;
    int u, v, weight;
    int x;
 
    bool done = false;
 
 
    while(!done)
    {
      cin >> nodes >> edges >> queries >> start;
      if(nodes == 0 && edges == 0 && queries == 0 && start == 0)
      {
        done = true;
      }
      else
      {
        Graph g(nodes);
 
        for(int i = 0; i < edges; i++)
        {
          cin >> u >> v >> weight;
          g.addEdge(u, v, weight);
        }
 
        dist = g.shortestPath1(start);
 
        for(int i = 0; i < queries; i++)
        {
          cin >> x;
          if(dist[x] == INFINITY)
          {
            cout << "Impossible" << endl;
          }
          else if(dist[x] == -INFINITY)
          {
            cout << "-Infinity" << endl;
          }
          else
          {
            cout << dist[x] << endl;
          }
        }
 
        cout << endl;
      }
    }
 
    return 0;
}
As I said, this is currently running in .5 seconds without calling shortestPath1. With shortestPath1, it runs for 5-8 seconds before the upload site kills it.

Disregard the bit about -Infinity, that's for a Bellman-Ford problem that uses a very similar setup.

Edit: You know what's weird? I'm actually working on Dijkstra's algorithm so I can do the opposite for the problem I'm working on - an algorithm that finds the LONGEST paths between two points. I did that basically by replacing some Infinitys with 0s and flipping a < sign - and when I upload that, it runs in .4 seconds. It apparently gets a wrong answer, but still. Frustrating. Here's that algorithm, if you're curious.

code:
vector<int> Graph::longestPath(int s)
{


  priority_queue<Path, vector<Path>, ComparePath> Q;
  vector<int> dist(this->numNodes, 0);
  Path p, q;
  int weight;


  p.n = &this->Nodes[s];
  p.distance = 0;
  Q.push(p);

  dist[s] = 0;

  while(!Q.empty())
  {
    p = Q.top();
    Q.pop();
    if(dist[p.n->index] == p.distance)
    {
      for(int i = 0; i < p.n->numAdjacent; i++)
      {
        weight = p.n->adjacentEdge[i].weight;
        if(dist[p.n->index] + weight > dist[p.n->adjacentTo[i]])
        {
          dist[p.n->adjacentEdge[i].to] = dist[p.n->index] + weight;
          q.n = &this->Nodes[p.n->adjacentEdge[i].to];
          q.distance = dist[p.n->adjacentEdge[i].to];
          Q.push(q);
        }
      }
    }
  }

  return dist;
}

Spiritus Nox fucked around with this message at 02:17 on Dec 6, 2012

chglcu
May 17, 2007

I'm so bored with the USA.
Not familiar with the algorithm, but is ComparePath maybe returning the inverse (or whatever the word is; like returning a - b when you should have b - a) of the correct value, causing your priority queue to have the wrong value on top?

Spiritus Nox
Sep 2, 2011

prolecat posted:

Not familiar with the algorithm, but is ComparePath maybe returning the inverse (or whatever the word is; like returning a - b when you should have b - a) of the correct value, causing your priority queue to have the wrong value on top?

I don't think so. My test values get the right answers either way and the website got wrong answers either way.

EDIT OF TRIUMPH: SOLVED IT, gently caress YEAH. :rock: There was just a case I wasn't accounting for in my driver that was giving me the wrong output to pass the LongestPath problem. ShortestPath still says I'm taking too long, but that's not what I'm getting graded on so if my algorithm looks good to you lot, forget it.

Spiritus Nox fucked around with this message at 02:54 on Dec 6, 2012

Jabor
Jul 16, 2010

#1 Loser at SpaceChem
Why are you computing distances for the entire graph, when you're only being asked about a subset?

If I were writing a test and wanted to trip people up, I'd use an incredibly complex graph where most of it is irrelevant, to ensure that they're not doing more work than necessary.

Spiritus Nox
Sep 2, 2011

Because I thought it would ultimately be less efficient to run a chunk of Dijkstra's algorithm 10 times than to run the whole thing once to find 10 nodes? Other students have passed without breaking it early.

But like I said above, I got LongestPath to pass and that's the one that counts. Thanks for the help, all of you.

Jabor
Jul 16, 2010

#1 Loser at SpaceChem

Spiritus Nox posted:

Because I thought it would ultimately be less efficient to run a chunk of Dijkstra's algorithm 10 times than to run the whole thing once to find 10 nodes? Other students have passed without breaking it early.

You don't need to throw away the work you've already done when it comes time to start the next one :ssh:

rjmccall
Sep 7, 2007

no worries friend
Fun Shoe
Oh, this was dumb. Popping a priority queue pulls out the highest-priority object, but you want to process lower weights before higher weights — i.e. lower weights should be higher priority. Your condition is indeed backwards.

Also, you cannot actually just run Dijkstra's with inverted conditions to solve the longest path problem. The best known solution to longest-path isn't asymptotically better than brute-force search. Your current implementation will happily execute a brute force search, but it'll also infinite-loop if there's a cycle in the graph.

Spiritus Nox
Sep 2, 2011

rjmccall posted:

Oh, this was dumb. Popping a priority queue pulls out the highest-priority object, but you want to process lower weights before higher weights — i.e. lower weights should be higher priority. Your condition is indeed backwards.

Also, you cannot actually just run Dijkstra's with inverted conditions to solve the longest path problem. The best known solution to longest-path isn't asymptotically better than brute-force search. Your current implementation will happily execute a brute force search, but it'll also infinite-loop if there's a cycle in the graph.

So 'return e1.weight < e2.weight;' needs to be flipped to get Dijkstra's algorithm working properly? I think I might have tried that...

roomforthetuna
Mar 22, 2005

I don't need to know anything about virii! My CUSTOM PROGRAM keeps me protected! It's not like they'll try to come in through the Internet or something!

Spiritus Nox posted:

So 'return e1.weight < e2.weight;' needs to be flipped to get Dijkstra's algorithm working properly? I think I might have tried that...
Usually this sort of compare function uses "return e1.weight - e2.weight", because they sort by three conditions, equal / less / greater. If this is the same as a quicksort compare then what you're telling the algorithm with an x<y comparison is either greater (returns more than 0) or equal (returns 0), with no possibility of a 'less than' result, which might result in wrong sorting and slowness. (And algorithm confusion because x<y says "equal" then y<x says "greater")

And I think you might be supposed to be using "shift" rather than "pop" to take things off your queue, because push and pop operate from the same end (as a stack), push and shift operate from different ends. Though that doesn't really matter since it's a sorted queue anyway, so long as you sort it so the right things are at the end you're pulling from.

Note: I could be completely wrong about any of this, priority_queue is not something I've ever used, this is just things from general programming with STL and data structures.

MutantBlue
Jun 8, 2001

roomforthetuna posted:

Usually this sort of compare function uses "return e1.weight - e2.weight", because they sort by three conditions, equal / less / greater. If this is the same as a quicksort compare then what you're telling the algorithm with an x<y comparison is either greater (returns more than 0) or equal (returns 0), with no possibility of a 'less than' result, which might result in wrong sorting and slowness. (And algorithm confusion because x<y says "equal" then y<x says "greater")

Note: I could be completely wrong about any of this, priority_queue is not something I've ever used, this is just things from general programming with STL and data structures.

You definitely don't want to use a three-condition compare function with C++ ordered containers and algorithms. They require compare predicates that satisfy a strict weak ordering (the default is "less than").

The old C library functions qsort() and bsearch() do take a three-condition compare function pointer.

Ciaphas
Nov 20, 2005

> BEWARE, COWARD :ovr:


I'm at the end of my rope with what seems to be a C I/O problem. I've got a program that writes data to a temporary file during processing, then at the end of processing uses that data to write a header for the real output file, then appends the temporary file to the end. Usually this works fine.

Sometimes instead of appending the data from the temp file to the output file, it reads a load of zero-bytes instead, as much as three kilobytes worth (but it's not consistent), then (and only then) starts appending real data from the beginning of the temp file.

Unfortunately the bug doesn't express itself when compiled in debug, only when compiled with our opt flags (which is literally just -O2). :sigh:

Doesn't seem to matter what method I use for the appending--reopening the files and using fgetc/fputc, using fread/write for blocks of data, or even punting it to a system("cat ...") call, the same thing happens. It also doesn't seem to matter if I sleep after flushing and closing the files but before starting the append, in case the OS is trying to catch up or something. However, if I DO sleep the program, then take a look at the temp file in something else before the program wakes up (like 'od -xv -A d tempfile | less'), it works perfectly every single time, like Schrodinger's bloody cat or something. :psyduck:

So I tried opening and closing the temp file a second time in the program before it starts the append, and that works... sometimes. And no matter what, at no time does ferror() report anything (assuming I'm using C file i/o for the attempt, of course).

I know this is vague as hell, but does anyone have even a guess what might be happening, given that I can't seem to observe the temporary file without ruining the bug, and given that I can't debug for it?

astr0man
Feb 21, 2007

hollyeo deuroga
Can you post your code? Or at least the I/O parts?

Jabor
Jul 16, 2010

#1 Loser at SpaceChem
Unless your system is really silly you should still be able to set breakpoints and debug your program even when specifying -O2.

Ciaphas
Nov 20, 2005

> BEWARE, COWARD :ovr:


Jabor posted:

Unless your system is really silly you should still be able to set breakpoints and debug your program even when specifying -O2.

I guess my system is really silly then; breakpoints are ignored (though dbx says they're ok), the execution line indicated seems to be tangentially related at best with what the program's executing (far as I can tell from the ongoing output, anyway), and local values are all "<value unavailable>" whether in -g -O2 or just -O2.

I hate SunCC/Sun Studio so much :smith:

astr0man posted:

Can you post your code? Or at least the I/O parts?

Going from memory here because the code's elsewhere without net access, but it's pretty bog standard as far as my beginner self is aware:

C++ code:
#include <string>
#include <sstream>
#include <cstdio>
#include <sys/stat.h>
using namespace std;

// both of these assume the named files have been flushed and closed previously

void appendOutput(const string& outputFN, const string& tempFN)
{
   const size_t BUF_SIZE = 512; // doesn't seem to matter much
   char* buf = new char[BUF_SIZE];
   FILE* output = fopen(outputFN.c_str(), "a+"); // had w at first, oops
   FILE* temp = fopen(tempFN.c_str(), "r+");

   // "did the files open ok" checking goes here in the real code

   /*
   If I do this, everything works perfectly for some reason
   struct stat statbuf;
   stat(tempFN.c_str(), &statbuf);
   */

   size_t bytesRead = 0;
   do
   {
      bytesRead = fread((void*)buf, sizeof(char), BUF_SIZE, temp);
      size_t bytesWrit = fwrite((void*)buf, sizeof(char), BUF_SIZE, output);
      if (bytesRead != bytesWrit) { /* error stuff that i've never seen triggered */ }
   } while (bytesRead == BUF_SIZE);

   if (!feof(temp)) { /* more error stuff as above */ }
   delete[] buf;
   fclose(output);
   fclose(temp);
}

void appendOutputByCat(const string& outputFN, const string& tempFN)
{
   stringstream ss("");
   ss << "cat " << tempFN << " >> " << outputFN; // hurr this is readable hurr

   /*
   If I do this, everything works perfectly for some reason
   struct stat statbuf;
   stat(tempFN.c_str(), &statbuf);
   */

   system(ss.str().c_str());
}
Oh yeah I forgot to mention, before I left today I found out that statting the temp file before I open it for reading (or run the cat) makes the whole thing work perfectly. What the hell. :(

Ciaphas fucked around with this message at 03:12 on Dec 7, 2012

Qwertycoatl
Dec 31, 2008

Ciaphas posted:

code:
      bytesRead = fread((void*)buf, sizeof(char), BUF_SIZE, temp);
      size_t bytesWrit = fwrite((void*)buf, sizeof(char), BUF_SIZE, output);

If fread doesn't give you all the bytes you asked for, you'll still write out an entire buffer, including whatever happened to be in buf before. The BUF_SIZE in your fwrite call should be bytesRead.

Qwertycoatl fucked around with this message at 22:11 on Dec 7, 2012

mobby_6kl
Aug 9, 2009

by Fluffdaddy

prolecat posted:

Do you maybe have a virus scanner set to scan files on access? Just a wild guess, and I haven't seen this particular problem, but in the past I've had Visual Studio have weird interactions with virus scanners. Just a possibility to check out.

Nope, no virus scanner. This actually turned out to be ever weirder than that though. Even when everything was seemingly ok, I'd sometimes actually have an outdated build running when debugging or testing, and as a result, ended up agonizing for a few hours over a bug that no longer existed.

Deleting everything from the Debug folder seems to make this issue go away for a while. I suspected a RAM/disk problem but everything besides VS works fine so maybe it's just this install that is broken.

Anyway, this is pretty much my first C++ project and...
  1. code:
    vector<int> v { 2, 3, 5, 7, 11, 13, 17 };
    Error	1	error C2601: 'v' : local function definitions are illegal
    
    Dammit Microsoft :negative:
    Luckily, C arrays still exist.

  2. What's the consensus on the native unit testing tool that's now built in? Would it work fine for this smallish project, or are 3rd party solutions still better?

Suspicious Dish
Sep 24, 2011

2020 is the year of linux on the desktop, bro
Fun Shoe

mobby_6kl posted:

code:
vector<int> v { 2, 3, 5, 7, 11, 13, 17 };
Error	1	error C2601: 'v' : local function definitions are illegal

You forgot an =

Vanadium
Jan 8, 2005

No, initializing stuff with just { values here } is a fancy universal C++11 thing. You can even do int i { 42 };.

Adbot
ADBOT LOVES YOU

Suspicious Dish
Sep 24, 2011

2020 is the year of linux on the desktop, bro
Fun Shoe
Did they really not want to type = or?

  • 1
  • 2
  • 3
  • 4
  • 5
  • Post
  • Reply