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

Make prox notation consistent #180

Merged
merged 6 commits into from
Jan 19, 2022
Merged

Make prox notation consistent #180

merged 6 commits into from
Jan 19, 2022

Conversation

Michael-T-McCann
Copy link
Contributor

Closes #75 .

@codecov
Copy link

codecov bot commented Jan 18, 2022

Codecov Report

Merging #180 (b2246cd) into main (73d300b) will not change coverage.
The diff coverage is 96.42%.

❗ Current head b2246cd differs from pull request most recent head bab7472. Consider uploading reports for the commit bab7472 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##             main     #180   +/-   ##
=======================================
  Coverage   92.21%   92.21%           
=======================================
  Files          48       48           
  Lines        3352     3352           
=======================================
  Hits         3091     3091           
  Misses        261      261           
Flag Coverage Δ
unittests 92.21% <96.42%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
scico/loss.py 88.67% <85.71%> (ø)
scico/functional/_functional.py 89.70% <100.00%> (ø)
scico/functional/_indicator.py 100.00% <100.00%> (ø)
scico/functional/_norm.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 73d300b...bab7472. Read the comment docs.

@bwohlberg bwohlberg added the documentation Improvements or additions to documentation label Jan 18, 2022
@Michael-T-McCann Michael-T-McCann marked this pull request as ready for review January 19, 2022 02:15
lam: Proximal parameter :math:`\lambda`.
kwargs: Additional arguments that may be used by derived
classes. These include ``x0``, an initial guess for the
minimizer.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps worth providing a bit more details on "the minimizer"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, x0 shouldn't be mentioned at all in many of these cases. In fact, kwargs is not often used, but I'm not sure how better to document that.

@@ -97,7 +97,7 @@ def prox(
lam: Proximal parameter :math:`\lambda`.
kwargs: Additional arguments that may be used by derived
classes. These include ``x0``, an initial guess for the
minimizer.
minimizer in the defintion of :math:`\mathrm{prox}`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Typo: defintion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed and flyspell-prog-mode added to init.el

@@ -51,7 +51,7 @@ def prox(
self, v: Union[JaxArray, BlockArray], lam: float = 1.0, **kwargs
) -> Union[JaxArray, BlockArray]:
r"""Evaluate the scaled proximal operator of the indicator over
the non-negative orthant:
the non-negative orthant, :math:`I`,:
Copy link
Collaborator

Choose a reason for hiding this comment

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

On its own, perhaps "I" might be confused for the identity matrix? Perhaps write something like I_{[\mb{x} > 0]} to explicitly include the set of which it is the indicator?

Copy link
Collaborator

@bwohlberg bwohlberg left a comment

Choose a reason for hiding this comment

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

Approved modulo minor additional comments.

@Michael-T-McCann Michael-T-McCann enabled auto-merge (squash) January 19, 2022 18:44
@Michael-T-McCann Michael-T-McCann merged commit 02776ac into main Jan 19, 2022
@Michael-T-McCann Michael-T-McCann deleted the mike/prox_notation_75 branch January 28, 2022 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve consistency of notation for proxes
2 participants