brunyuriy / adasim Goto Github PK

Since we generate a directed graph, the randomization at the moment produces 
only one-way streets and it is extremely unlikely that by accident a street 
becomes two way.
I'm pretty sure that that is not what we want, even though it is not a big 
technical problem. Except that cars cannot turn around and go back home ;-)

Using the default package is generally a bad idea. Move the main class to the 
top level of your own package structure, so "traffic".

Nearly all unit tests fail in the 'schema' branch. You MUST fix your code (and 
possibly some of the tests). Most of the failures relate to unexpected 
exceptions being thrown. 
The goal of these modifications is:
- make all tests pass (all tests check for valid conditions, so none may be 
removed or modified in ways that change their meaning)
- make sure that when an incorrect configuration is encountered, a proper 
message is printed to the console (issue #32)

Update the JavaDoc in the source (of at least all classes you wrote):

1. Write a reasonably detailed description of what a class is for.
2. Write a reasonably detailed description of what a method does for *all* 
public, protected and default methods.
3. Write additional comments of what methods do for all methods that contain 
more than 5 lines of code/10 statements.

Make GraphNode an active element in the Simulation by making it implement 
AdasimAgent.

Create a directory that contains all the different test inputs.
For example create a directory
"resources/test"
and put all the different config files that you use for testing there.
This will help keep the project better organized and might even avoid clutter.

Create a generator for random simulation setups.
The generator takes the following inputs:
- number of nodes
- chance for a pair of nodes to have a link between them
- average delay per node (picked from a uniform distribution or something)
- number of cars
- a list of car strategies. For each car a strategy is picked randomly from the 
list.
- a filename for the output file

The generator creates an XML configuration file with the randomly generated 
information.

The functionality you implement in setGraph() constructs a Graph object. As 
such it should be part of the Graph class, either as a constructor or an 
explicit init() method.

I've added a number of test cases for all kinds of incorrect inputs. The names 
of the test methods and the input files should give you a good idea what the 
tests are for.

There is a number of different things:
- Tests for stuff that can safely be ignored (for example car with invalid 
start or end nodes
- Test for stuff that breaks the simulation (for example no graph, cars, nodes 
defined)
- Tests for other incorrect stuff in the config files (for example non-existing 
strategies etc)

In all cases you should check for these error conditions. All cases also should 
report a WARNING message (through the logger) when stuff is being ignored, and 
an ERROR message followed by program termination when stuff breaks the 
simulation.
Especially the ERROR conditions might be better handled higher up and thus 
should throw an exception that is handled by main() or something.

You can extend the test cases to check for correct logging output by using the 
massert stuff (https://bitbucket.org/jowu/massert)

http://code.google.com/p/adasim/wiki/XML_config should say for each of the 
"strategy" elements that this element needs to implement the such and such 
interface, and link to the .java file in the source (or the javadoc api maybe?).

At the moment all classes you create know how to parse their bit of 
configuration file (the Car knows how to parse the position string, the graph 
how to parse the graph file). This is VERY bad, because every time you make a 
change to the config file you may have to touch these classes. It is also bad 
design, because the Car should not need to be constructed from a config file at 
all.
To fix this:

1. Create a class SimulationFactory that knows how to read config files and 
constructs the simulation objects. There's the AbstractFactory pattern, read up 
on that!

2. Change the classes that need to be configured so that the constructor takes 
concrete values. For example, the Car constructor should look like
Car( int startPosition, int endPosition, int carID )

3. Additionally you have to make the strategies configurable, so either add 
another parameter to the constructor, or also create setters for the Strategy 
and Noise objects.

4. For fixing this problem with the Graph, there is issue #4.

The capacity function should depend on:
1. the weight of the node,
2. the cutoff at which traffic starts to affect the node,
3. the number of cars on the node.

The current implementation of Dijkstra's Algorithm fails when we try to compute 
paths from node x to node y and there are no paths.

I attached "random.xml" (not minimized) that exhibits this behavior.

A lot of the elements and attributes in the XML config files are required. 
Add test cases that check for correct and expected behavior when this 
information is not there.
In most cases this should terminate the program with a meaningful error message.

Examples:
id, start, end are all required for cars
id, neighbors are required for nodes

Currently the generator always supplies the LinearSpeedStrategy.
It should eventually have the option to generate a random selection from a set 
of strategies, pretty much the same is is currently supported for CarStrategies.

Use the auto-generator to generate a large graph and lots of cars and see if 
the whole thing becomes impossibly slow. If a simulation with 1000 nodes and 
100 cars runs more than 1 minute there's a problem.

Scale to
1000 nodes /100 cars
10000 nodes / 1000-5000 cars

What happens if we have more cars than nodes?

If the configuration is invalid, the user should see a pretty error message 
about what is wrong, not an ugly exception.  

A lot of the conditions we are currently checking manually could be checked by 
the parser if we had a DTD. That would simplify our code significantly.

The minimum should be 1 node.  What if someone wants to make sure their 
algorithms do not blow up on a one-node graph.  I don't think it's very 
interesting, but it's still a map with a single street.  It's a valid map.  

It would be nice to have a simple mechanism to have the ant build automatically 
generate a JAR that contains the version number that we also have as the 
version when we release builds.

This version number should also be used by Version.java for logging to the 
console.

The delay for each node should be configurable. 

Right now, we have to unpack the XSD file before Adasim can use it. 
This is inconvenient for distributing the thing and it should not be hard to 
make this transparent.

Running the ant build currently gives a warning 
{{{ [javac] /Users/gorla/Documents/adasim/adasim/build.xml:29: warning: 
'includeantruntime' was not set, defaulting to build.sysclasspath=last; set to 
false for repeatable builds}}}

This is not a real problem, but it would be nice if it doesn't show up.

The TrafficSimulator class should be configure with a list (or set) of agents 
and work only with those. What kind of agent they are should be complete 
transparent.

In the "cars" and "nodes" nodes, define attributes for default strategies, so 
the strategy only needs to be specified explicitly on nodes/cars that differ 
from the default. 

What is the difference between makePath and redoPath? Shouldn't the return 
exactly the same result given the same starting and ending nodes?

Use and explicit ID attribute for cars and nodes.
That attribute should then be stored in the id field of the corresponding 
objects when loaded.

1. Create a new source directory called "test"
2. Replicate the package structure you have for the main program
3. Move all tests to the matching packages. For example, for the class 
traffic.model.Car in directory src/, the test goes to traffic.model.CarTest in 
directory test/

In TrafficSimulator, the field carNum has the class invariant

carNum == cars.size()

For that reason, I do believe it is a good idea to remove this field, use 
cars.size() where appropriate and use a local variable when you have to do 
computations to update things (this happens in readPositions())

This is a minor refactoring aimed at making the implementation more consistent 
with the terminology in the paper we wrote

To query the simulation for nodes and cars we need an interface that returns 
them based on their ID instead of their position in a the internal list. 
It might be useful to consider using a different internal data structure to do 
this.

What's the purpose of the Path interface? It appears inconsistent, because it 
combines methods that are needed to perform simulation steps (tryMove()) and 
methods that most likely should be private to Cars, which are the only class 
that implement that interface anyway.

Suggestions:
1. Make sure this interface has a clear purpose, document that purpose and 
refactor it to match this purpose.
2. OR simply delete this interface.

The is a simple refactoring the locates data where it belongs. We might either 
deprecate CarInfo later, or turn it into the filtering interface for noise and 
privacy.

Move all fields except the noise strategy.

The old interface that loads configurations from text files should be removed.
Eliminate all classes, methods, tests and input files that relate to that (and 
check that the rest of the project still correctly builds with that).

The simulation when no path from "start" to "end" for a car can be found.
In this case the CarStrategy returns null and subsequent uses of the path do 
not check for "null".

At least one example is in Car.moveCar()

This should be safe and fail gracefully (at least say that there is a car for 
which no path was found).

To ease compilation for other people, we should provide an ant build script (at 
the least). Possibly even a maven build file (even though that might be 
overkill right now).

The name runSim() seems to suggest that calling this method will run the whole 
simulation. However, that is not the case. runSim() only makes on step in time. 
So a better name would be takeSimulationStep() or something along these lines.

Take a known graph (for example the one from the Wikipedia page on Dijkstra's 
algorithm), translate the edge-weigthed graph into a node-weighted graph and 
write a number of test cases that check if the algorithm correctly computes the 
costs. 
Don't forget test cases for errors like asking for a path between two nodes 
that don't exist, for nodes that are not connected, etc.

CarInfo.getCurrent() : should be getCurrentPosition()
the same for Car
CarInfo.getFinish(): should be atDestination()
the same for Car
CarInfo.setStart(): setStartNode()
CarInfo.setEnd(): setTargetNode() or setEndNode()
CarInfo.getLimit(): what limit is that?

Car.tryMove() seems to be analogous to runSim(): maybe rename it to something 
like takeSimulationStep()?

Generally, give good names to classes and methods. Make sure they talk about 
WHAT the methods and classes do. Also, make sure you write at least a short 
JavaDoc comment for public methods. Ideally, you do document all methods.

Right now, all traffic affects roads directly. But we should be able to model 
roads with different capacities, like highways and little neighbourhood roads. 
For this, we should add a "capacity" to the road. That would be the number of 
cars that does not affect the maximum speed on the road. So, for example if the 
capacity of a road is 10, the having 5 cars on the road does not slow down 
traffic at all, but having 12 cars on the road has some effect.


This issue must be fixed before issue #26.

To make the simulation for flexible and allow more interesting scenarios, we 
need something that can modify the simulation while it is running.
For example, we'd like to be able to add cars some time during the simulation, 
instead of having them all start at the same time. Other things that require 
some form a central controller are creating the effects of accidents and other 
road closures.

This is a major feature addition that requires several things:
  # Implement the central controller and integrate it into the main simulation loop. This central controller must be able to schedule events to happen at a given time (iteration) in the simulation.
  # Modify the structure of the configuration file. This should contain for now the possibility to schedule the addition of cars at some time later instead of the start of the simulation. I think that the best way to do it (to keep the config file simple), is to add an optional attribute to cars that defines the time when they start. If not provided, cars start at the beginning.

A lot of last minute stuff got added in a very hackish way.
This needs to be reworked/removed for a release version.

All strategy attributes should accept a fully qualified classname to be loaded 
by reflection instead of hard-coding the mapping from shortcuts like "R" or "D" 
to some implementation.

One can imagine a graph with nodes with only incoming edges.  There is no way 
to get out once you get in there, but there is no reason to disallow this.  It 
may be of interest to someone to experiment with such graphs' effects on 
algorithms.  

Once we can configure agents in the XML file, we want to be able to make them 
part of the simulator. 
The simulator constructor should be updated to accept a list of agents in 
addition to the paramters it already takes, and the XMLReader has to make sure 
it passes an appropriate list to the constructor.

The wiki needs to describe the output format.

At the moment all SpeedStrategies compute the additional delay only based on 
the number of cars currently on the node. This does not reflect the fact that 
different roads have different capacities (be they two lanes or just very long 
so a number of cars can be spaced further apart.
We should think of intelligent ways to model reality here.

What's wrong with single isolated nodes?  It may be of interest to someone to 
experiment with such graphs' effects on algorithms.  

To be able to incorporate more different agents into the simulation, we have to 
add more stuff to the config file format.
Define a new <agents> node at the same level as <cars> and <graph>, and an 
<agent> element with two atributes:
'class' should be the fully qualified name of the class that implements the 
agent
'parameters' is a string that will be passed to the agent after it is 
instantiated. This string is intended for agent specific parameters that cannot 
be put into the configuration file.

We need to make sure the capacities are working correctly, particularly around 
each of the parameter values (e.g., when 
capacity = the number of cars, 
capacity+1 = the number of cars, 
capacity-1 = the number of cars.