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

Enable multiple edges in the same direction between two nodes for pandas #156

Closed
wants to merge 6 commits into from

Conversation

jsfreischuetz
Copy link
Contributor

@jsfreischuetz jsfreischuetz commented Jul 15, 2024

Enable multiple edges in the same direction between two nodes

Description

Currently the solver does not support multiple edges of between two nodes in the same direction. This draft PR shows a quick way of enabling this feature in Pandas. This requires changing some minor implementation details about the way that indices are used in the DataFrame when converting.

fixes #155

For networkx, it would require a type change to MultiDiGraph, and for scipy, it may not be trivially possible to implement these changes.

This PR draft is mostly intended as a jumping off point for showing it is possible to solve this, rather then a general solution.

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:
    • Background and problem specification
    • Example of the input data format (use gurobi_optimods.datasets for loading data)
    • Runnable code example
    • Presentation of solutions
    • Included in the mod gallery and toctree

@CLAassistant
Copy link

CLAassistant commented Jul 15, 2024

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@jsfreischuetz jsfreischuetz marked this pull request as ready for review July 15, 2024 22:42
@jsfreischuetz
Copy link
Contributor Author

@simonbowly Would it be possible to trigger the test cases. They are working locally, but I want to make sure that everything is working properly.

@simonbowly
Copy link
Member

@jsfreischuetz thanks for the work! I re-ran the tests. The issue I have is that this changes the expected input/output data format. To make sure the PR doesn't make backwards-incompatible changes, could you please revert the original tests and add some new tests for cases with duplicate edges (and their expected results)?

This should still be do-able, pandas can handle duplicate values in the index.

@jsfreischuetz
Copy link
Contributor Author

@jsfreischuetz thanks for the work! I re-ran the tests. The issue I have is that this changes the expected input/output data format. To make sure the PR doesn't make backwards-incompatible changes, could you please revert the original tests and add some new tests for cases with duplicate edges (and their expected results)?

This should still be do-able, pandas can handle duplicate values in the index.

The current format returns only identified the edge with a tuple of source destination for each flow. This makes it ambiguous which edge was taken. It can be reconstructed by the user by making a few assumptions about the flows.

I will make the change leaving the format, but for ease of use for users, eventually the output format should likely be modified.

@simonbowly
Copy link
Member

The current format returns only identified the edge with a tuple of source destination for each flow. This makes it ambiguous which edge was taken. It can be reconstructed by the user by making a few assumptions about the flows.

Good point, it might need a re-think.

@torressa do you have any thoughts on this one?

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.

Two edges between two nodes not properly calculated
3 participants