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

ADMM extensions #29

Merged
merged 8 commits into from
Oct 7, 2021
Merged

ADMM extensions #29

merged 8 commits into from
Oct 7, 2021

Conversation

bwohlberg
Copy link
Collaborator

  • Add an iteration timer
  • Add an optional callback function for the solve method

@bwohlberg bwohlberg added the enhancement New feature or request label Oct 7, 2021
@@ -7,6 +7,8 @@

"""ADMM solver and auxiliary classes."""

from __future__ import annotations
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this another case of

 # needed to annotate a class method that returns the encapsulating class
 # see https://www.python.org/dev/peps/pep-0563/

? If so, suggest adding the comment. Otherwise people might assume this is needed in every source file.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, and agreed.

scico/admm.py Outdated
@@ -383,6 +387,7 @@ def __init__(
and an ADMM object, responsible for constructing a tuple ready for insertion into
the :class:`.diagnostics.IterationStats` object. If None, default values are
used for the tuple components.
callback: An optional callback function that
Copy link
Contributor

Choose a reason for hiding this comment

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

"that..." something?

scico/admm.py Outdated
self.maxiter: int = maxiter
self.timer: Timer = Timer()
# ToDo: a None value should imply automatic selection of the solver
Copy link
Contributor

Choose a reason for hiding this comment

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

This TODO seems slightly stale, or maybe it would be better as a feature request issue.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed. See #31.

@bwohlberg bwohlberg merged commit 1ee471b into main Oct 7, 2021
@bwohlberg bwohlberg deleted the brendt/admm-ext branch October 7, 2021 20:39
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.

2 participants