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

Setup support for multiple algorithms and add ALS #21

Closed
Tracked by #6
dahong67 opened this issue Oct 12, 2023 · 0 comments · Fixed by #26
Closed
Tracked by #6

Setup support for multiple algorithms and add ALS #21

dahong67 opened this issue Oct 12, 2023 · 0 comments · Fixed by #26
Assignees

Comments

@dahong67
Copy link
Owner

dahong67 commented Oct 12, 2023

Currently, the only algorithm we offer is LBFGSB - it will be great to support other algorithms. And with the changes in #13, it should now be possible to handle default algorithms in an elegant and extensible way.

Idea change definition of gcp to be:

gcp(X::Array, r, loss = LeastSquaresLoss(); alg = default_algorithm(X, r, loss)) = _gcp(
    X,
    r,
    loss,
    alg,
)

where default_algorithm(X, r, loss) applies some heuristics to choose a default algorithm (e.g., ALS for LeastSquaresLoss, etc), and we add some algorithm types that have fields holding their parameters (with default values chosen):

  • LBFGSB
  • ALS

Note: thinking it's a good idea to remove the type restriction on loss to prevent duplication in LossFunctionsExt - losses from LossFunctions.jl will simply get routed through this signature too. Means we'll need to move that dispatch logic into _gcp, which is probably more sensible anyways (separates the user-friendly signature that has defaults from the dispatching logic). [UPDATE 10/12/2023: addressed in #22]

A couple more ideas:

  1. Could have LossFunctionsExt simply introduce a wrapper type LossFunctionsLoss <: AbstractLoss that wraps loss functions from LossFunctionsExt which could unify things even more.
  2. Could also consider explicitly enumerating the actual concrete LossFunctions.jl types supported in LossFunctionsExt. The current code defines the domains on abstract types, which seems reasonable and applies for current losses but may not apply for all subtypes in the future. Needs some more thought.
@dahong67 dahong67 mentioned this issue Oct 12, 2023
26 tasks
@dahong67 dahong67 self-assigned this Oct 12, 2023
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 a pull request may close this issue.

1 participant