Skip to content

Conversation

@aymuos15
Copy link
Contributor

@aymuos15 aymuos15 commented Jan 3, 2026

I have set it up so one can feed the solver for Transport/Birkhoff projections via make_solver argument. @carlosgmartin for this PR is this enough (as the tests are namesake for these) or should I only port the sparse_simplex for now?

Continuation of #1280, #1351 and #1439

@carlosgmartin
Copy link
Contributor

Thanks!

I'd ask one of the project maintainers, perhaps @rdyro, about the desired API.

Copy link
Collaborator

@rdyro rdyro left a comment

Choose a reason for hiding this comment

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

Thanks!

@vroulet
Copy link
Collaborator

vroulet commented Jan 15, 2026

It looks pretty good to me. Just to be sure @aymuos15 :

  • what changes did you do compared to jaxopt? None?
  • did you take all tests?
  • did you check if you kept optax formatting of e.g. docstrings? I think references are not well formatted (compare to optimizers). You can build the docs locally to see if the docs look good.

Thanks again!

@aymuos15 aymuos15 force-pushed the feat/add-more-jaxopt-projections branch from f4a0b8c to 55523dc Compare January 15, 2026 18:12
@aymuos15
Copy link
Contributor Author

I did just push a fix for the docstrings. Thank you very much for pointing that out.

@vroulet For simplex, I have made sure we are matching everything. For transport and birkhoff, they are usable with an outside solver.

Transport/Birkhoff projections require a solver via make_solver argument.
@aymuos15 aymuos15 force-pushed the feat/add-more-jaxopt-projections branch from 55523dc to c8e85fc Compare January 15, 2026 18:16
@vroulet
Copy link
Collaborator

vroulet commented Jan 16, 2026

Ok, would it be possible to make two separate PRs? One for the sparse simplex projection and one for the transport/birkhoff projections.
This one can be the sparse simplex.

The problem with the transport/birkhoff is that (i) it requires a solver, (ii) the api for the solver is not documented and it takes a specific form like solver.run(...).params. Such api choices need to be documented. If we add these projections I would actually use a default solver rather than letting the user select. Right now, there is no "solver" in optax that could fit the requirements

@aymuos15
Copy link
Contributor Author

Okay, that's a very fair argument. Thank you. I will change this to just the sparse simplex. Instead of raising another PR right away, should I first raise a separate issue to discuss what the default solver should be (or continue the convo in one of the other issues)?

@vroulet
Copy link
Collaborator

vroulet commented Jan 22, 2026

@aymuos15 you may raise the issue and open a discussion with amybe @carlosgmartin if he's interested.

For now, just prune this PR from the Birkhoff and transport projections. I'm happy to merge after that.

Thanks again!

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.

4 participants