-
Notifications
You must be signed in to change notification settings - Fork 17
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
Clean up example scripts #26
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a nice improvement in consistency. Some of the capitalization is not what I would default to, but I don't have an argument against it.
xt[idx] = np.random.rand(s) | ||
y = D @ xt + 5e-2 * np.random.randn(m) # synthetic signal | ||
y = D @ xt + 5e-2 * np.random.randn(m) # Synthetic signal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: update style guide to note this convention. Also update style guide to follow this convention.
@@ -76,14 +77,16 @@ def demosaic(cfaimg): | |||
""" | |||
s = Afn(img) | |||
rgbshp = s.shape + (3,) # Shape of reconstructed RGB image | |||
nsigma = 2e-2 # Noise standard deviation | |||
σ = 2e-2 # Noise standard deviation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a style guideline? I hope not: I would have to search the web to discover how to type these characters in my editor, I imagine the same is true for others. I don't think the "cool factor" is worth it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it's not a style guideline, but it's already used quite extensively in the examples. Agreed on the extra pain in entering the characters, but I think it does enhance readability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Emacs-- C-x 8 Enter
should open up the unicode search list, or M-x insert-char
.
Our style guide says no unicode in public APIs. They are allowed in internal APIs. The user should never have to use unicode chars.
In this case, the user can choose to use unicode symbols or not; they aren't required.
|
||
In SCICO, switching between these two regularizers is a one-line change: | ||
replacing an [L1Norm](../_autosummary/scico.functional.rst#scico.functional.L1Norm) with a [L21Norm](../_autosummary/scico.functional.rst#scico.functional.L21Norm). | ||
where $R$ is the isotropic TV: the sum of the norms of the gradient vectors at each point in the image $\mathbf{x}$. The same reconstruction is performed with anisotropic TV regularization for comparison; the isotropic version shows fewer block-like artifacts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line length okay?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is another issue that should be addressed in the style guide. At the moment, docstrings in examples often have very long lines. One could argue that it doesn't matter because they are primarily going to be viewed in markup-rendered form.
@@ -34,7 +29,7 @@ | |||
""" | |||
N = 256 # Image size | |||
|
|||
# these steps create a ground truth image by spatially filtering noise | |||
# These steps create a ground truth image by spatially filtering noise |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Example comments tend to be imperative, this isn't. Suggestion:
"Create a ground truth image by spatially filtering noise." (note full stop)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. There are probably other examples like this that I also didn't catch,
""" | ||
Denoise with isotropic total variation | ||
""" | ||
reg_weight_iso = 2e0 | ||
f = loss.SquaredL2Loss(y=y) | ||
g_iso = reg_weight_iso * functional.L21Norm() | ||
|
||
# the append=0 option makes the results of horizontal and vertical finite differences | ||
# The append=0 option makes the results of horizontal and vertical finite differences |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"...differences." (full stop)
""" | ||
Denoise with anisotropic total variation for comparison. | ||
""" | ||
# we tune the weight to give the same data fidelty as the isotropic case | ||
# We tune the weight to give the same data fidelty as the isotropic case |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Tune the weight..." full stop
fig.show() | ||
|
||
# zoomed version | ||
# Zoomed version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
full stop
examples/scripts/pgm_tv_isotropic.py
Outdated
@@ -38,7 +34,8 @@ | |||
""" | |||
N = 256 # Image size | |||
|
|||
# these steps create a ground truth image by spatially filtering noise | |||
|
|||
# These steps create a ground truth image by spatially filtering noise |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Create a ground truth..." full stop.
Clean up example scripts