Adds more documentation and examples for `padarray`, `Fill` and `Pad` #54

zygmuntszpak · 2018-01-17T03:59:36Z

No description provided.

zygmuntszpak · 2018-01-17T07:12:39Z

@timholy I'm working my way through the code base trying to improve the documentation by adding explanations and examples. When you have a moment, please could you review this pull-request and also take a look at #53 ?

codecov · 2018-01-17T13:03:24Z

Codecov Report

Merging #54 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master      #54   +/-   ##
=======================================
  Coverage   90.84%   90.84%           
=======================================
  Files           8        8           
  Lines        1070     1070           
=======================================
  Hits          972      972           
  Misses         98       98

Impacted Files	Coverage Δ
src/border.jl	`96.87% <100%> (ø)`	⬆️

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 0186e50...0341977. Read the comment docs.

zygmuntszpak · 2018-01-17T13:04:29Z

Found a typo and fixed an emdash in my original commit.

zygmuntszpak · 2018-01-24T09:15:20Z

@timholy bump :)

kmsquire

Generally looks good! I have one small change request and a couple of comments. If possible, it would be great if @timholy took a quick look, but I can merge if he doesn't get to it soon.

kmsquire · 2018-01-24T21:02:05Z

src/border.jl

+
+#### Usage illustration
+Use `Pad(:replicate,2,2)` to designate that the top and bottom border should be
+replicated by two pixels, and the left and right border by four pixels.


Should be Pad(:replicate, 2, 4) to match the text.

Done—Thank you!

kmsquire · 2018-01-24T21:07:57Z

src/border.jl

-
-It's worth emphasizing that padding is most straightforwardly specified as a string,
-
-    imfilter(img, kernel, "replicate")


I guess that this information is available in the imfilter docs, but wondering if it's also useful here still.

I removed it because there isn't actually any method in Pad or Fill which takes a string. It seems odd to mention it here, since it is a description of how to use the imfilter API. I am in the process of reworking and extending the imfilter documentation in a different branch.

kmsquire · 2018-01-24T21:24:05Z

src/border.jl

+6 & 6 & 6 & 6 & 6  & 12  & 18  & 24  & 30  & 36  & 36  & 36  & 36  & 36 \\\\
+6 & 6 & 6 & 6 & 6  & 12  & 18  & 24  & 30  & 36  & 36  & 36  & 36  & 36 \\\\
+6 & 6 & 6 & 6 & 6  & 12  & 18  & 24  & 30  & 36  & 36  & 36  & 36  & 36 \\\\
+6 & 6 & 6 & 6 & 6  & 12  & 18  & 24  & 30  & 36  & 36  & 36  & 36  & 36


These examples look really nice when rendered, but unfortunately, at the REPL, the raw latex is returned (try ?padarray at the julia prompt). I'm not sure if there's a nice way around this.

I posted a question about this at https://discourse.julialang.org/t/showing-latex-docstrings-at-the-repl/8573.

I think the solution to this conundrum is to implement software to render latex to ascii-art so that mathematics can be printed in the console. See this post for some examples.

Here is another good example.

I have started working on a Julia package to do this type of thing. If I works out, then perhaps it can be incorporated into Base.

In the meantime, I have adjusted the indentation of the matrices in the docstring so that when they are printed in the console they align well and are more visually pleasing. You should find the latest commit more readable.

Thanks for posting the question on discourse. There is a precedent for having Latex in docstring in Base. See ?fft for example.

timholy

Sorry for the long absence. This and your other documentation improvements are truly extraordinary, so nice that I'm definitely going to merge it.

Bigger picture, the only thing I worry about (can't remember if I brought this up before) is that experienced users might often use ?imgradients just to be reminded what arguments should be supplied in what order. Unfortunately with more documentation there's some scrolling involved to go back up to the top and see the signature. I wonder if Julia needs to have a notion of "brief docs" and "thorough docs", perhaps triggering the brief one with ??imgradients or something (note the 2 ? marks).

timholy · 2018-02-02T18:21:53Z

src/border.jl

+extrapolation scheme.
+
+You can specify a different amount of padding for the top, left, bottom and
+right border of an image.


I definitely think of 2d as the "default," but one of the features of JuliaImages is that it generalizes to 3d and 4d imaging (e.g., biomedical imaging). I don't want to convey too strong of an impression that these constructs are 2d-only; conversely, I don't want to explain things in an unnecessarily complicated way for the majority of folks who are doing 2d image processing. Would something like "You can specify a different amount of padding at the lower and upper borders of each dimension of the image (top, left, bottom and right in two dimensions)" be acceptable?

It's fine if all of your examples are 2d.

I've followed your suggestion and also added some extra phrases to emphasize that the types support multi-dimensional arrays.

I've also added three-dimensional examples for Pad and Fill to the padarray function.

timholy · 2018-02-02T18:24:56Z

src/border.jl

+refer to the documentation of  [`Pad`](@ref).
+
+You can specify a different amount of padding for the top, left, bottom and
+right border of an image.


timholy · 2018-02-02T19:12:38Z

src/border.jl

+    Fill(value::T, lo::Dims{N}, hi::Dims{N})
+```
+Construct an instance of [`Fill`](@ref) designating a `value`  and tuples
+`lo` and `hi` which stipulate the number of row and columns which will be predended


Thanks for spotting that---fixed.

timholy · 2018-02-02T19:14:49Z

src/border.jl

+
+"""
+```julia
+    Fill(value, lo::AbstractVector, hi::AbstractVector)


Here you say lo is an AbstractVector but the text talks about a tuple. The code accepts either. Perhaps just don't put a type on these arguments? Then you only need to document one of them. Here too you could alternatively use the style

Fill(value::T, lo::Dims{N}, hi::Dims{N}) Fill(value::T, lo::AbstractVector, hi::AbstractVector) Construct an instance...

timholy · 2018-02-02T19:19:24Z

src/border.jl

    Pad(style::Symbol, (m,n))
+```
+Construct an instance of [`Pad`](@ref) such that the image is prepended and appended symmetrically with `m` pixels at the lower and upper edge of dimension 1, and `n` pixels for dimension 2.


One option you can consider is to write the docstring like this:

""" Pad(style::Symbol, m, n, ...) Pad(style::Symbol, (m, n, ...)) Construct an instance of ... """

One of the things I like about this style is that is reduces the amount of text that users have to read, because the main explanation is stated only once.

But this is your choice, and I'll merge whatever you prefer.

I followed your suggestion.

zygmuntszpak · 2018-02-07T09:51:17Z

Thanks for the great feedback and useful suggestions on the pull-request. I've tried to incorporate all your suggestions, and added text to emphasize that padding is also supported on multi-dimensional arrays. I've also included some 3D examples in padarray to ensure that people can see at a glance that the scope of the function and types extends beyond 2D.

I agree that a 'brief docs' would be useful when working in the REPL. In IJulia one automatically gets a scroll window, so one can still see the function signature. Similarly, Atom's documentation browser also ensures that the function signature appears. The problem is definitely the REPL and your ?? suggestion could help address the problem.

zygmuntszpak · 2018-02-14T12:48:07Z

@timholy Just a reminder that I've amended this pull request with the changes you requested.

zygmuntszpak · 2018-02-21T09:26:43Z

@kmsquire Hi Kevin, I think it might be a while before Tim has a moment to return to this thread. Would you mind merging this pull-request?

kmsquire · 2018-02-21T15:01:15Z

Hi Zygmunt, I would be happy to, but unfortunately, there are merge conflicts right now. I might be able to resolve them myself when I get to a computer, but if you see this before I do, can you take a look?

zygmuntszpak · 2018-02-22T11:13:37Z

@kmsquire I've pulled Julia master into my forked repository, and then merged my up-to-date forked repository with my padarray branch. I didn't experience any merge conflicts. I've pushed this latest version, so presumably there should be no conflicts since it matches the latest version of master.

zygmuntszpak · 2018-02-26T10:53:46Z

@kmsquire ping :)

kmsquire · 2018-02-27T05:12:58Z

Sorry, haven't had many spare moments recently. Thanks for the additional ping, and for the contribution!

zygmuntszpak mentioned this pull request Jan 17, 2018

Adds Bickley and Scharr gradient filter and improves imgradient docs #53

Merged

zygmuntszpak force-pushed the padarray branch from bd4ccdf to fa56b80 Compare January 17, 2018 13:03

kmsquire requested changes Jan 24, 2018

View reviewed changes

zygmuntszpak force-pushed the padarray branch from fa56b80 to ceb2fd1 Compare January 30, 2018 12:15

timholy reviewed Feb 2, 2018

View reviewed changes

Adds more documentation and examples for padarray, Fill and Pad

1124c36

zygmuntszpak force-pushed the padarray branch from ceb2fd1 to 1124c36 Compare February 7, 2018 09:37

timholy mentioned this pull request Feb 7, 2018

Brief and extensive docs with ? and ?? JuliaLang/julia#25930

Closed

Adds more documentation and examples for padarray, Fill and Pad

0341977

Evizero mentioned this pull request Feb 26, 2018

Expands imfilter documentation with more details and examples #56

Merged

kmsquire merged commit c8e041b into JuliaImages:master Feb 27, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds more documentation and examples for `padarray`, `Fill` and `Pad` #54

Adds more documentation and examples for `padarray`, `Fill` and `Pad` #54

zygmuntszpak commented Jan 17, 2018

zygmuntszpak commented Jan 17, 2018

codecov bot commented Jan 17, 2018 •

edited

Loading

zygmuntszpak commented Jan 17, 2018

zygmuntszpak commented Jan 24, 2018

kmsquire left a comment

kmsquire Jan 24, 2018

zygmuntszpak Jan 30, 2018

kmsquire Jan 24, 2018

zygmuntszpak Jan 30, 2018

kmsquire Jan 24, 2018

zygmuntszpak Jan 30, 2018

timholy left a comment

timholy Feb 2, 2018

zygmuntszpak Feb 7, 2018

timholy Feb 2, 2018

zygmuntszpak Feb 7, 2018

timholy Feb 2, 2018

zygmuntszpak Feb 7, 2018

timholy Feb 2, 2018

zygmuntszpak Feb 7, 2018

timholy Feb 2, 2018

zygmuntszpak Feb 7, 2018

zygmuntszpak commented Feb 7, 2018 •

edited

Loading

zygmuntszpak commented Feb 14, 2018

zygmuntszpak commented Feb 21, 2018

kmsquire commented Feb 21, 2018

zygmuntszpak commented Feb 22, 2018

zygmuntszpak commented Feb 26, 2018

kmsquire commented Feb 27, 2018


		It's worth emphasizing that padding is most straightforwardly specified as a string,

		imfilter(img, kernel, "replicate")

Adds more documentation and examples for padarray, Fill and Pad #54

Adds more documentation and examples for padarray, Fill and Pad #54

Conversation

zygmuntszpak commented Jan 17, 2018

zygmuntszpak commented Jan 17, 2018

codecov bot commented Jan 17, 2018 • edited Loading

Codecov Report

zygmuntszpak commented Jan 17, 2018

zygmuntszpak commented Jan 24, 2018

kmsquire left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timholy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zygmuntszpak commented Feb 7, 2018 • edited Loading

zygmuntszpak commented Feb 14, 2018

zygmuntszpak commented Feb 21, 2018

kmsquire commented Feb 21, 2018

zygmuntszpak commented Feb 22, 2018

zygmuntszpak commented Feb 26, 2018

kmsquire commented Feb 27, 2018

Adds more documentation and examples for `padarray`, `Fill` and `Pad` #54

Adds more documentation and examples for `padarray`, `Fill` and `Pad` #54

codecov bot commented Jan 17, 2018 •

edited

Loading

zygmuntszpak commented Feb 7, 2018 •

edited

Loading