mac icon indicating copy to clipboard operation
mac copied to clipboard

Major refactor, API changes, new tests

Open keevindoherty opened this issue 1 year ago • 0 comments

Summary

This PR introduces a major project restructure, some API changes, and new tests.

At a high level, the new project structure looks something like this:

mac
├── LICENSE
├── benchmarks /* python scripts for benchmarking */
├── data /* sample datasets */
├── docs  /* doc files, compiled docs */
├── examples  /* example scripts using MAC, including for pose-graph SLAM */
├── mac  /* actual library code; this is what you 'pip install' */
│   ├── optimization
│   ├── solvers
│   └── utils
├── requirements.txt
└── tests. /* test code; mirrors the library structure */
    ├── optimization
    ├── solvers
    └── utils

I won't go into detail about the content that is consistent with what is on main, but in terms of the new files, we have:

  • benchmarks is for Python performance benchmarks. These are not meant to be unit tests, but instead report on solution quality and timing statistics for a few representative datasets.
  • mac/optimization is for the core optimization routines we use. This includes the Frank-Wolfe implementation in frankwolfe.py and implementation of linear program oracles for a variety of compact, convex constraint sets in constraints.py.
  • mac/utils is a directory containing what was formerly in the utils.py and cholesky_utils.py files. These are now split up into separate utility files conversions.py for NetworkX < > MAC format conversions, graphs.py for Laplacian constructors and general graph manipulation utils, rounding.py for rounding to the feasible sets implemented in constraints.py, and cholesky.py for Suitesparse Cholesky support. I also moved fiedler.py containing our custom hooks into the NetworkX TRACEMIN-Fiedler implementation here, but I might move these to a separate fiedler directory along with the specialized Cholesky utilities specific to Fiedler value computation.
  • mac/solvers contains implementations for three solvers: MAC, GreedyESP [Khosoussi et al. 2019] - a greedy algorithm for maximizing the (reduced) Laplacian determinant, and "GreedyEig" which is a greedy algorithm for optimizing algebraic connectivity.

Bugs squashed

  • In frankwolfe.py the relative duality gap convergence criterion sets a threshold of the form (dual_upper_bound - f(x)) / f(x) < relative_duality_gap_threshold. This will fail if f(x) happens to be zero, and will "false positive" if f(x) is negative. We fix this by moving the f(x) in the denominator to the right-hand side and taking the absolute value. So we now compute (upper - f(x)) < relative_duality_gap_threshold * f(x). This is more robust numerically, but of course if f(x) is indeed zero, it will never trigger. I think this is OK for now. The fix would likely be to add some absolute duality gap threshold, but this seems pretty hard to specify, since we do not know the "scale" of f(x).
  • In conversions.py, there were a variety of subtly bugs converting between NetworkX graphs and the MAC edge list format. These were mostly because the code was "overfit" to the specific case where the NetworkX graphs we were interested in were all unweighted (or had weights uniformly equal to one). This is fixed now.

Testing

New tests added to tests/... including:

  • Tests for the core optimization libraries, running Frank-Wolfe on some simple constrained concave maximization problems (e.g. quadratic objective). We now explicitly test cases like f(x) \approx 0 which would have caused a "divide by zero" error before (see above)
  • Tests for utils libraries. We now test for connected / disconnected graphs in our Fiedler value computations. The latter test (disconnected graph) is currently failing. We need to fix this to improve support for features onkjd/no-fixed-graph, but we actually have a test now! We also test our routines for converting between NetworkX and MAC, which actually had some subtle bugs (see above).

keevindoherty avatar Aug 04 '24 20:08 keevindoherty