Skip to content

Conversation

@Rishav159
Copy link
Contributor

This PR adds bi-directional Search diagram. The main motivation here is to provide intuition on why bi-directional search is faster than standard bfs.
The diagram has side by side comparison for the performance of both the algorithms.
Progress on #57

@redblobgames
Copy link
Contributor

It's not working for me in either chrome or firefox or safari; can you try jslint/jsbeautify to see if there are any errors in the new files?

@Rishav159
Copy link
Contributor Author

Oops! My bad. I made a syntax error at the end moment. Fixed it.

}
}

class BidirectinalProblem {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Bidirectional" is misspelled here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops! Will fix it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

}
}

class BreadthFirstSearch {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've already written breadth first search for chapter 3; is it possible to reuse that, or make the other version compatible with this one?

Copy link
Contributor Author

@Rishav159 Rishav159 Jul 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The earlier Breadth First Search was simply a 3 line code that takes the problem object and returns the first element from its frontier. Here, the Breadth First Search class performs what the class GraphAgent performed earlier. But in this particular case, I am using generators. Generators prove to be especially useful here since there are 2 bfs running simultaneously and then there is one diagram that extracts nodes from the envelopeiterate()function from Bi-directional Problem.
Additionally, the implementation of the graphs is also different. For example, the getAdjacent function earlier simply scanned the edges list to get the nodes adjacent to a node; but in this case, that will be too costly since there are thousands of nodes. So here, we keep a list of adjacent nodes in the graph node itself. Similarly, there are more fundamental differences between the implementations. Due to all this reasons, I thought trying to reuse the code would get in my way of making the diagram.
What are your thoughts on this part?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, ok, that makes sense

}
return [nodes, edges];
}
class Graph {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've already written a graph class for the other parts of the page; is it possible to reuse that here? Otherwise we will have two incompatible graph classes, two incompatible breadth first searches, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We did not have a separate class for graph. We simply had a DefaultGraph class which returned the small graph we used in other visualizations.

return [nodes, grid];
}

function poissonDiscSampler(width, height, radius) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the license on this code?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It comes under gpl-3.0 license. Got it from https://gist.github.com/mbostock/dbb02448b0f93e4c82c3

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will add the license in the footer. Would that be fine ? Or should I try to implement my own version of the algorithm?
Actually I tried that and for some reason couldn't get it right. After a complete day wasted on it, I decided to go for the implemented version.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, sorry, adding the license would not be enough. The short version is: in order to use other people's code, even open source, you have to follow all the rules, and GPL has different rules than the MIT license. Adding the license isn't enough to follow the GPL rules. There are several issues here.

  1. Any code you check into the repository is by default copyrighted by you. (Berne Convention and other laws). If you checked this in, you would be claiming that you are the author. But you are not the author. This is true even of open source. We would need to put this into a third-party/ folder along with the copyright and license information, so that we are making it clear that it is not code you wrote, and you are not claiming copyright over it.
  2. Open source code is copyrighted just like any other code. You cannot use copyrighted code by default. The open source license grants permission to use it under certain requirements. You can only use it in if you follow those requirements.
  3. The license lists the conditions under which you may use it. The AIMA code is under the MIT license. The poisson disc code is under the GPL 3 license. Both of these licenses require that they retain the original copyright and license, but poisson_distribution.js does not do either. That means it's in violation of the license and it cannot be used. Adding the license and putting it into a third-party/ folder would meet this requirement.
  4. GPL also requires that all other source code in the program also be released under the GPL license. That means if you check in poisson_distribution.js, the rest of the project has to be GPL. That means we would need to go to each of the contributors to aima-javascript and ask them to re-release the code under the GPL license. So adding the license and putting it into third-party/ is not enough to use GPL code, unless we re-license everything else as GPL, and I don't think we're going to do that.

That said, it is a good idea to use libraries. We just have to use them in ways they allow. You could either write a separate program that uses the GPL library and writes out JSON, and then you would mark that program as GPL also (without making the rest of the project GPL), or you could use a library with a different license, such as poisson-disk-sampling, which is under the MIT license.

I think the cleanest thing to do would be to write a separate program that runs in node.js that uses the poisson-disk-sampling library, and have it write out JSON.

A simpler option would be to make a grid instead of the poisson disk graph. Offset grids (every other row is shifted by 1/2 column) and jittered grids (every point is moved by a small random amount) would provide enough variety to make the graph look nice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had no idea there are so much restrictions to even open source code. This rules make sense though. Thanks for the information !
I will either generate a JSON using the poisson-disk-sampling library or try the gittered grid. Whichever works better, we can use.

@Rishav159 Rishav159 force-pushed the biDirectional branch 2 times, most recently from b9f8d73 to 5accc87 Compare July 22, 2017 18:05
@Rishav159
Copy link
Contributor Author

I updated the PR and replaced the poisson-disc-sampling code with gittered-graph implementation. The code got a lot simpler.

@redblobgames
Copy link
Contributor

Yay simpler code!

Suggestions:

  1. I'm not sure if you do this already but when picking the random start/end point, make sure they're at least some distance apart to show the effect you want. The easiest way to do this is to always use the same start/end point (are different start/end points adding to the explanation?). Another way to do it is to always pick a start point in the left 1/3rd of the diagram and an end point in the right 1/3 of the diagram.
  2. The visual effect is seeing circles grow over time. The speed of the growth is based on the area. But to the viewer, it feels slower and slower, because the perceived speed is based on the radius, which is sqrt(area). To compensate for this, I think you should try increasing the speed over time. You can do this in the interval function — instead of always increasing steps by 1, decide how many steps will be run this update. It might be Math.floor(1 + 0.1 * Math.sqrt(this.steps)) steps (you may need to try different values instead of 0.1). Then loop through the bfs iterator that many times, then at the end draw everything.
  3. Instead of drawing text into the canvas, put a
    underneath the canvas and draw text there. It will better match the text of the page. Also, this ;-)

@Rishav159
Copy link
Contributor Author

There maybe some performance issues with this one. The speed you see right now is the maximum speed I could manage! I used canvas, and even tried to pre-run the algorithm and then draw later (as with the backtracking diagram). Still, I couldn't make it faster than it is right now.
One way to do this might be to reduce the number of nodes.

@redblobgames
Copy link
Contributor

For performance, the setInterval is currently set to 1000 redraws per second even though most screens do not redraw more than 60 times per second. We can get slightly better performance by using a larger interval. Here's something that worked reasonably on my system (but may not be fast enough — try it out)

  bfs() {
      this.iterator = this.problem.iterate();
      this.steps = 0;
      this.nodeFrom = 'source';
      
      this.intervalFunction = setInterval(() => {
          let numSteps = Math.floor(1 + 0.5 * Math.sqrt(this.steps));
          let next;

          for (let step = 0; step < numSteps; step++) {
              next = this.iterator.next();
              if (next.done) {
                  clearInterval(this.intervalFunction);
                  break;
              }
              let node = next.value;
              if (node !== this.initial && node !== this.final) {
                  //If node is not initial or final, mark it as explored and fill blue
                  this.context.fillStyle = this.edgeColor;
                  this.context.beginPath();
                  this.context.arc(this.nodes[node].x, this.nodes[node].y, 1.5, 0, 2 * Math.PI, true);
                  this.context.fill();
                  this.context.closePath();
              }

              for (let i = 0; i < this.nodes[node].adjacent.length; i++) {
                  let adjacentNode = this.nodes[node].adjacent[i];
                  this.context.beginPath();
                  this.context.lineWidth = 1;
                  this.context.strokeStyle = (this.nodeFrom == 'source') ? this.initialColor : this.finalColor;
                  this.context.moveTo(this.nodes[node].x, this.nodes[node].y);
                  this.context.lineTo(this.nodes[adjacentNode].x, this.nodes[adjacentNode].y);
                  this.context.stroke();
                  this.context.closePath();
              }
              this.nodeFrom = (this.nodeFrom == 'source') ? 'destination' : 'source';
              this.steps++;
          }
          
          //Update label
          this.context.clearRect(0, this.h - 20, this.w - 20, 20);
          this.context.font = "25px Comic Sans MS";
          this.context.fillStyle = "black";
          this.context.textAlign = "center";
          this.context.fillText(`Nodes Generated: ${this.steps}`, this.w / 2, this.h);
      }, 20);
  }

Note that your BidirectionalDiagram and BFSDiagram have some similar code, and you'll have to make these same changes in BFSDiagram. Or better, find a way to share that code so that you don't have to make this kind of change in both classes.

@redblobgames
Copy link
Contributor

If you do fix the start/end points, and maybe even if you don't, you might consider putting a large obstacle in the middle of the graph, like figure 25.18. It might be more interesting, but on the other hand it might also make the main point harder to see.

@redblobgames
Copy link
Contributor

Visually, the diagram does show the main concept for bidirectional search, but it looks different from the other diagrams on the page. Have you tried it with coloring the circles but not changing the color of the edges, like the breadth first search diagram earlier on the page? The circles would have to be a little larger than they are now. If that works you'd be able to reuse the same colors and styling (just with smaller circles) so that readers would be able to relate the new diagram to the previous ones.

@Rishav159
Copy link
Contributor Author

Will try it now

@Rishav159
Copy link
Contributor Author

Updated the PR to match the color code with the previous diagrams (in accordance with the PR #95).
Also, removed generator functions and replaced with normal functions to improve performance.

@redblobgames redblobgames merged commit 9fd4613 into aimacode:master Jul 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants