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

Migrate flow algorithms from GraphsFlows #329

Closed
wants to merge 5 commits into from
Closed

Migrate flow algorithms from GraphsFlows #329

wants to merge 5 commits into from

Conversation

gdalle
Copy link
Member

@gdalle gdalle commented Jan 17, 2024

Take everything in GraphsFlows.jl and put it in Graphs.jl.

This is half of the solution to #108

  • remove the min cost flow part, which relies on JuMP.jl (see GraphsOptim.jl)
  • copy-paste the code to src/flows
  • copy-paste the tests to test/flows
  • copy-paste the docs and put it in docs/src/flows.md
  • modify src/Graphs.jl and test/runtests.jl and docs/make.jl to include necessary files
  • format everything
  • modify docs to include private names in a separate section
  • modify nearly all tests to use generic graphs

@matbesancon since it's your code do you have anything to say about this?

@gdalle gdalle requested a review from matbesancon January 17, 2024 15:10
Copy link

codecov bot commented Jan 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (5878e7b) 97.30% compared to head (b158054) 97.41%.
Report is 7 commits behind head on master.

❗ Current head b158054 differs from pull request most recent head 3ff0588. Consider uploading reports for the commit 3ff0588 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #329      +/-   ##
==========================================
+ Coverage   97.30%   97.41%   +0.10%     
==========================================
  Files         115      124       +9     
  Lines        6796     7259     +463     
==========================================
+ Hits         6613     7071     +458     
- Misses        183      188       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@gdalle gdalle added enhancement New feature or request do not merge Do not merge this PR (yet) labels Jan 17, 2024
@gdalle
Copy link
Member Author

gdalle commented Jan 29, 2024

Tests are failing because type inference in 1.6 is less good than in 1.10. They didn't use to in the original repo, presumably because of generic graphs. @simonschoelly maybe you could help?

@matbesancon
Copy link
Member

I'm good with it yup thanks!

@gdalle
Copy link
Member Author

gdalle commented Feb 2, 2024

This is the part that is not well inferred for GenericGraph

@traitfn residual(flow_graph::::IsDirected) = Graphs.DiGraph(Graphs.Graph(flow_graph))

@gdalle gdalle removed the do not merge Do not merge this PR (yet) label Feb 7, 2024
@simonschoelly
Copy link
Member

Can I suggest that we move everything initially to the Experimental submodule. This way

  • We won't have any name clashes as long as we don't export anything.
  • We don't initially guarantee that the API for flows is stable so we still can't change things
  • It is easier to track what we change from GraphFlows.jl when we do this in a separate PR instead of doing it in this one, where we don't have a history.

@gdalle
Copy link
Member Author

gdalle commented Feb 11, 2024

That's a good idea but as for tracking changes, beyond renaming I only did the bare minimum so that the code could run. A lot of initial changes are gonna be necessary even in the Experimental submodule

@gdalle
Copy link
Member Author

gdalle commented Feb 11, 2024

Also I'm a bit lazy at the prospect of redoing all of these necessary changes from scratch just so we can track the renaming, but I understand your point

@gdalle
Copy link
Member Author

gdalle commented Mar 5, 2024

Closing this to reopen a cleaner PR separating the copypaste from the actual changes to GraphsFlows

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants