JOSS Review: Benchmarks

torressa commented 4 years ago

Points related to benchmarks from: https://github.com/openjournals/joss-reviews/issues/2408#issuecomment-651542971

[x] Running the benchmarks requires pandas and matplotlib, but pandas and matplotlib are not listed as dependencies (or at least not on the main page)
[x] Running pytest benchmarks/ failed many of the tests on my Windows computer? The examples worked and the tests passed.

Points related to benchmarks from: https://github.com/openjournals/joss-reviews/issues/2408#issuecomment-655272567

Structure

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.

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

Import Errors in `tests/benchmarks/`

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

[x] 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

[x] test_ortools error is

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

[x] In test_toy.py, remove "from pytest import raises" --not used. same for test_greedy
[x] In cvrp_augeratpy, remove "from vrpy.main import VehicleRoutingProblem" --not used
[x] In benchmarks folder, some files have a main() function, some files don't. Why?

Cordeau

[x] Running mdvrp_cordeau.py fails with raise KeyError("Node %s missing from initial solution." % v) KeyError: 'Node 52 missing from initial

Who? @torressa @Halvaros

torressa commented 4 years ago

We've already made a start to this to test the diving heuristic (renaming the pricing strategies and adding the lost solve method for Augerat) We are also thinking of adding a page to the docs with the results.

However, (not urgent!) it'd be good if you could explain how you run the different Augerat instances @Kuifje02:

Do you run all the sets A, B and P?
What's the best vrpy set up greedy=True, maximum 1 iteration?

That way we can see if diving actually increases the number of optimal solutions found

torressa commented 4 years ago

Realised I broke the greedy initial solution for the new column based formulation in dev...

torressa commented 4 years ago

Fix benchmark tests, still have to rework pandas and matplotlib dependencies.

torressa commented 4 years ago

Realised I broke the greedy initial solution for the new column based formulation in dev...

Fixed now, I think...

Kuifje02 commented 4 years ago

Running the benchmarks requires pandas and matplotlib, but pandas and matplotlib are not listed as dependencies (or at least not on the main page)

yes for pandas (for reading the data files), but matplotlib can be removed if I am not mistaken. Was there initially to plot the graph. I showed how to do this in the jupyter notebook here, which will need an update as well.

Add benchmarks page to docs with comp results per benchmark

This is definitely a good idea. I was waiting for results to be competitive :D

torressa commented 4 years ago

Nice man!!

torressa commented 4 years ago

Most of the import errors have been sorted in the Halvaros/dev branch, will check when merged into dev. And I think the Cordeau has also been removed in the joss branch.

torressa commented 4 years ago

We need to have a discussion about the structure point. Whether moving the tests in tests/benchmarks into the examples folders and the other stuff in #52

Kuifje02 commented 4 years ago

I am thinking maybe it would make sense to have everything related to benchmarks in a one independent folder, and only usage examples in the examples folder. So as suggested in #52, maybe :

benchmarks/ |_data/ |_cvrp_augerat |_cvrptw_solomon ++ @Halvaros' new scripts and files ++ maybe the tests that are in the tests/benchmarks right now ? (not sure)

torressa commented 4 years ago

That split makes more sense! Moving the test files there also makes sense.

torressa commented 4 years ago

Working on the restructuring and path fixing thing... Big commit dropping soon

torressa commented 4 years ago

Last one:

Running pytest benchmarks/ failed many of the tests on my Windows computer? The examples worked and the tests passed.

Anyone on a \ Windows\ machine pls

torressa commented 4 years ago

Still have to maybe move the ortools related tests to the examples folder maybe

Kuifje02 commented 4 years ago

Anyone on a \ Windows\ machine pls

I am on Windows. When I run the tests (from \vrpy) I get :

_______________________________ ERROR collecting benchmarks/tests/test_cvrp_augerat.py ________________________________
ImportError while importing test module 'C:\Users\romain.montagne\Desktop\vrpy\benchmarks\tests\test_cvrp_augerat.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
benchmarks\tests\test_cvrp_augerat.py:1: in <module>
    from benchmarks.augerat_dataset import AugeratDataSet
E   ModuleNotFoundError: No module named 'benchmarks'

and similarly with the other tests. You think this is Windows specific?

Kuifje02 commented 4 years ago

Still have to maybe move the ortools related tests to the examples folder maybe

Yes I agree! + this would satisfy the JOSS suggestion to assert the results of the examples (https://github.com/Kuifje02/vrpy/issues/53#issue-653177840)

torressa commented 4 years ago

Not sure.. :thinking: Are you doing python3 -m pytest benchmarks/?

torressa commented 4 years ago

Without all the sys paths you need to either do that or add an empty __init__.py file. (see here)

Kuifje02 commented 4 years ago

Not sure.. 🤔 Are you doing python3 -m pytest benchmarks/?

Actually I was just doing pytest benchmarks/. If I do python3 -m pytest benchmarks/, nothing happens (no error is raised, but no test is run either):

Indeed, pytest benchmarks/ is not the right way:

Kuifje02 commented 4 years ago

Ok, I think I got it... for some reason, although I am using python 3, (on Windows ?) I need to run python -m pytest benchmarks (and not python3 ...)

On unix/os, is python3 mandatory ? Or does python -m pytest benchmarks work ?

torressa commented 4 years ago

Not sure, I've always used it cause python calls python2.7. But I think it might be a an installation thing, installing python3.whatever after python2.whatever, so the name is already taken

Kuifje02 / vrpy