Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Min-Cost Flow Problem #39

Merged
merged 18 commits into from
Apr 27, 2023
Merged

Min-Cost Flow Problem #39

merged 18 commits into from
Apr 27, 2023

Conversation

torressa
Copy link
Member

@torressa torressa commented Mar 10, 2023

Description

Implements min-cost flow from #11.

Checklist

  • Implementation:
    • Implementation of the Mod in the gurobi_optimods installable package
    • Tests for the Mod implementation in tests/
    • Docstrings for public API, correctly linked using sphinx-autodoc
  • Documentation page:
    • Problem specification with description tab and optimisation model tab
    • Example of the input data format (use gurobi_optimods.datasets for loading data)
    • Runnable code example
    • Presentation of solutions

Have a nice day!

@torressa
Copy link
Member Author

torressa commented Mar 10, 2023

@stevedwards, @simonbowly I've found that we can use networkx.convert_matrix.to_scipy_sparse_array to get multiple edge attributes out (as independent matrices):

>>> import networkx as nx
>>> G = nx.DiGraph()
>>> G.add_edge(0, 1, weight=1.0, capacity=2.0)
>>> G.add_edge(1, 2, weight=3.0, capacity=4.0)
>>> print(nx.to_scipy_sparse_array(G, weight="weight"))
  (0, 1)        1.0
  (1, 2)        3.0
>>> print(nx.to_scipy_sparse_array(G, weight="capacity"))
  (0, 1)        2.0
  (1, 2)        4.0

Alternatively, we can add networkx directly (or use pandas), I don't mind, but it would be good to have an agreement between optiflowmods.

@torressa torressa marked this pull request as draft March 10, 2023 15:39
@torressa
Copy link
Member Author

torressa commented Apr 25, 2023

Hey @simonbowly could you please check if the idea of d994dd4 is OK?
I'm thinking of separating all of the mods that were initially in my network_flow file out into separate files (including docs).
I started with the weirdest one min_cost_flow. Following our discussion from #47 I think for this case it makes sense to keep functions separate (as the inputs are different for each case: pandas, scipy and networkx) and the users should be the ones taking care here. But as you said #47 (comment), this looks a bit weird so just want to be sure.

The other ones I will write in the same vein as your matching problem (a single entry point with hidden functions for different interfaces).

@simonbowly
Copy link
Member

This looks good to me. In the docs for min cost flow, the networkx example is very cluttered. You could make this a bit easier to read by:

  • Loading a networkx graph directly from datasets. There's no need to show the user how to convert from pandas to networkx if the mod will handle either case equally easily.
  • Remove the pos attributes from the graph, since they aren't required to solve the mod. Just include them when plotting.

It also might make sense to change the doc structure a bit. You don't have to stick to specification -> code -> solution exactly. In this case it might make more sense to have separate sections for each input format, or you could use synced tabs to switch all examples/output to be relevant to the input format the user wants.

@torressa torressa changed the title Network Flow Problems Min-Cost Flow Problem Apr 26, 2023
@torressa torressa marked this pull request as ready for review April 27, 2023 11:20
docs/requirements.txt Outdated Show resolved Hide resolved
docs/source/mods/min-cost-flow.rst Outdated Show resolved Hide resolved
docs/source/mods/min-cost-flow.rst Outdated Show resolved Hide resolved
docs/source/mods/min-cost-flow.rst Show resolved Hide resolved
docs/source/mods/min-cost-flow.rst Outdated Show resolved Hide resolved
tests/test_min_cost_flow.py Show resolved Hide resolved
src/gurobi_optimods/min_cost_flow.py Show resolved Hide resolved
@simonbowly
Copy link
Member

@torressa this is looking great! I added a few specific comments. Overall I think the documentation looks a little sparse - do you think it needs a bit more detail?

@torressa
Copy link
Member Author

I think I've addressed all the comments (not sure if marking them as resolved is what one does?).

I've fleshed out the formulation explanation a bit more. Would you like to see more content in the min-cost-flow.rst? I'm not sure I can say much more, maybe on a more inspired day.

@simonbowly
Copy link
Member

Nice one, thanks for the great work! Yeah I know the feeling, let's merge this and wait to see if inspiration strikes later :-)

@simonbowly simonbowly merged commit 9c62ee9 into Gurobi:main Apr 27, 2023
5 checks passed
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.

None yet

2 participants