JOSS Review : general comments

[Kuifje02]

Some of the items may be repeated in other issues. Just wanted to be sure we don't forget anything from openjournals/joss-reviews#2408 (comment).

• Out of the box, running tests in unittests/ folder from PyCharm (on Windows) worked for me. Test_toy took some time to run but maybe that's expected
  => Check and see if one of the tests is too long and change accordingly

• Related to JOSS Review: Benchmarks #42, fix the whole benchmarks item

• Out of the box, all tests in benchmarks/ fail. All errors, except the one in test_ortools, are the same:

with open(path + instance_name) as fp:
E FileNotFoundError: [Errno 2] No such file or directory: '../examples/benchmarks/data/P-n16-k8.vrp'
....\examples\benchmarks\cvrp_augerat.py:53: FileNotFoundError

• You seem to have an issue with setting path variables. Please check. test_ortools error is

from ortools.data import (
E ModuleNotFoundError: No module named 'ortools'

• Again, a similar issue. It can't find the ortools folder which is under examples/

• Running mdvrp_cordeau.py fails with

raise KeyError("Node %s missing from initial solution." % v)
KeyError: 'Node 52 missing from initial solution.'

• I suggest renaming the "ortools" folder. The current import statements read as if you have a dependency on the ortools library. It's not obvious that it's just a folder in the upper level.

• Some examples are using numpy.matrix(). It is not recommended to use. Please consider regular numpy ndarrays.
  => This is NetworkX dependent, needs investigation

• Some clean up

• There is a lot of mingling with path variable to append sys.path for folders. I don't think this is necessary and complicates the code/usage --since one now needs to track what's in the path and what's not. I don't see anything special in your library structure that would require altering with path variables. Cannot we resolve these import issues without hacking into the path?

• In test_toy.py, remove "from pytest import raises" --not used. same for test_greedy

• In cvrp_augeratpy, remove "from vrpy.main import VehicleRoutingProblem" --not used

• In benchmarks folder, some files have a main() function, some files don't. Why?

• In examples/ folder, you typically print the best value and best route. Please also ASSERT them. I can run them, but I don't know if the solution is correct or not.

• why is the separation between tests and examples. Why not add these examples to test and assert them.

• is there a way to turn off logging from the underlying solvers like CBC etc? CBC should have some verbosity flag. There are a lot of printouts that are not directly related to this library, and might be confusing for the uninterested user.

• examples.ipync is nice.. but please add ASSERTs along with prints() so that we can verify correctness.

• You might want to add LICENSE information as a header on top of every source code file. Maybe also add License section to README?

• Thinking out loud comment: if you rename main.py to vrp or vehicle_routing, then the imports would read:

from vrpy.vrp import VehicleRoutingProblem
from vrpy.vehicle_routing import VehicleRoutingProblem

which might read a bit more relevant than importing "main"

[Kuifje02]

I removed the examples/notebook, as everything is in the docs and the tests.