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

Add optimize package #122

Closed
Michael-T-McCann opened this issue Dec 6, 2021 · 1 comment · Fixed by #129
Closed

Add optimize package #122

Michael-T-McCann opened this issue Dec 6, 2021 · 1 comment · Fixed by #129
Assignees
Labels
enhancement New feature or request

Comments

@Michael-T-McCann
Copy link
Contributor

Michael-T-McCann commented Dec 6, 2021

With the addition of new optimizers, I find having each optimizer as a top-level module odd. I.e., looking at the table of contents for the API reference (https://scico--117.org.readthedocs.build/en/117/_autosummary/scico.html), we have scico.admm and scico.ladmm at the same level as scico.linop. I propose that it's time for a scico.optimizer package.

Originally posted by @Michael-T-McCann in #117 (comment)

From in person discussion: Names should be like scico.optimize.ADMM. scico.solver functions should move over to scico.optimize.

@bwohlberg bwohlberg added the enhancement New feature or request label Dec 6, 2021
@bwohlberg
Copy link
Collaborator

bwohlberg commented Dec 6, 2021

Copied from discussion in #117:

That's a good suggestion, but there are a few questions to resolve regarding the implementation:

  • Naming:
    • Instead of optimizer, I would go with optimize, for consistency with scipy.optimize.
    • Are we going to also move scico.solver into this subpackage? If not, the name should perhaps reflect specific properties that differentiate it from the functions in scico.solver, e.g. scico.proxopt, since all the algorithms that would be within the subpackage would be proximal algorithms.
  • Structure. Do we
    • retain the existing modules, so that e.g. scico.admm.ADMM becomes scico.optimize.admm.ADMM, or
    • follow the same approach as in e.g.scico.linop, so that scico.admm.ADMM becomes scico.optimize.ADMM.

My leaning is towards naming it scico.optimize, which would include scico.solver, and making the classes direct members of this module, e.g. scico.optimize.ADMM.

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 a pull request may close this issue.

2 participants