Skip to content

feat: PathFrankWolfe structs#229

Merged
louise-poole merged 2 commits into
mainfrom
split-routing/tnl/ENG-5853-pfw-structs
Jun 5, 2026
Merged

feat: PathFrankWolfe structs#229
louise-poole merged 2 commits into
mainfrom
split-routing/tnl/ENG-5853-pfw-structs

Conversation

@tamaralipows
Copy link
Copy Markdown
Contributor

@tamaralipows tamaralipows commented Jun 2, 2026 •
edited
Loading

Task

  • Assumed expects are okay in computation_requirements since all algorithms have this and this is only at startup
  • Remove "fraction" from param names max_probe and min_split. It was confusing that it said fraction but was f64

@tamaralipows tamaralipows force-pushed the split-routing/tnl/ENG-5853-pfw-structs branch from 8991599 to c667bed Compare June 2, 2026 16:02
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jun 2, 2026

No API Breaking Changes Detected

The PR title signals breaking changes, but cargo-semver-checks found none.
If the breaking change is behavioral, CLI, or config-level (not public Rust API), this is expected.
Otherwise, consider using fix: instead of feat: in the PR title.

@tamaralipows tamaralipows force-pushed the split-routing/tnl/ENG-5853-pfw-structs branch from c667bed to 99d55a9 Compare June 2, 2026 16:29
dianacarvalho1
dianacarvalho1 reviewed Jun 3, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@dianacarvalho1 dianacarvalho1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @tamaralipows ! 🙏🏼 I have a concern

Comment thread fynd-core/src/worker_pool/registry.rs Outdated
@tamaralipows tamaralipows force-pushed the split-routing/tnl/ENG-5853-pfw-structs branch from 63984f1 to f78e247 Compare June 3, 2026 18:01
louise-poole
louise-poole reviewed Jun 4, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@louise-poole louise-poole left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! A couple minor suggestions here quick

Comment thread fynd-core/src/algorithm/path_frank_wolfe.rs
Comment on lines +37 to +39
/// Split-routing algorithm that discovers multiple paths via Bellman-Ford
/// and optimises the input split across them using a Frank-Wolfe loop.
pub struct PathFrankWolfeAlgorithm {
Copy link
Copy Markdown
Collaborator

@louise-poole louise-poole Jun 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it's easy enough to make this generic over the algorithm... 👀 I suppose that's a huge refactor because we're using BellmanFord specific fns like solving a single route etc...
[no action required, just thinking out aloud]

Copy link
Copy Markdown
Contributor Author

@tamaralipows tamaralipows Jun 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it would need a trait like SingleRouteAlgorithm that exposes build_context + find_single_route - those are BF-specific today and not part of Algorithm. Doable but not worth it until we have a second algorithm that could also serve as a path finder imo. Want to avoid premature generalization for now.

Copy link
Copy Markdown
Collaborator

@louise-poole louise-poole Jun 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is indeed out of scope. I'm just thinking ahead to the Algorithm competition project - this could be a useful tool for those designing a new algorithm.

Comment thread fynd-core/src/algorithm/path_frank_wolfe.rs Outdated
Comment thread fynd-core/src/algorithm/path_frank_wolfe.rs Outdated
Comment thread fynd-core/src/algorithm/path_frank_wolfe.rs Outdated
Comment thread fynd-core/src/algorithm/path_frank_wolfe.rs Outdated
@tamaralipows tamaralipows force-pushed the split-routing/tnl/ENG-5853-pfw-structs branch 2 times, most recently from c6b06ac to 7ff2d81 Compare June 4, 2026 14:51
@tamaralipows tamaralipows self-assigned this Jun 4, 2026
louise-poole
louise-poole approved these changes Jun 5, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@louise-poole louise-poole left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! 👍

tamaralipows and others added 2 commits June 5, 2026 14:50
@tamaralipows @claude @louise-poole
feat: PathFrankWolfe structs
4bb5511 
- Assumed expects are okay in `computation_requirements` since all
  algorithms have this and this is only at startup (same with
  new_with_defaults)
- Remove "fraction" from param names `max_probe` and `min_split`. It was
  confusing that it said fraction but was f64

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@tamaralipows @claude @louise-poole
refactor: simplify algorithm constructors
6e651ca 
Also, mark PFW solve as unimplemented

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@louise-poole louise-poole force-pushed the split-routing/tnl/ENG-5853-pfw-structs branch from 7ff2d81 to 6e651ca Compare June 5, 2026 12:50
@louise-poole louise-poole enabled auto-merge June 5, 2026 12:50
@louise-poole louise-poole merged commit cd62e18 into main Jun 5, 2026
15 checks passed
@louise-poole louise-poole deleted the split-routing/tnl/ENG-5853-pfw-structs branch June 5, 2026 12:53
@propellerci
Copy link
Copy Markdown

propellerci Bot commented Jun 5, 2026

This PR is included in version 0.74.0 🎉

@propellerci propellerci Bot added the true label Jun 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Troshchk Troshchk Troshchk left review comments

@dianacarvalho1 dianacarvalho1 dianacarvalho1 left review comments

@louise-poole louise-poole louise-poole approved these changes

Assignees

@tamaralipows tamaralipows

Labels

true

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@tamaralipows @louise-poole @Troshchk @dianacarvalho1