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

Document the generic functions nextind() and prevind() #52658

Merged
merged 8 commits into from Jan 28, 2024

Conversation

JamesWrigley
Copy link
Contributor

@JamesWrigley JamesWrigley commented Dec 28, 2023

While going through the comments on #52625 I noticed that the array methods for nextind() and prevind() didn't have docstrings, only the string method.

Fixes #42084.

@jishnub
Copy link
Contributor

jishnub commented Dec 29, 2023

Are these methods useful for arrays? Typically, axes for arrays are AbstractUnitRanges, so indices should always be spaced by one

@JamesWrigley
Copy link
Contributor Author

I think they're mostly useful for writing generic code that could take in arbitrary indices. e.g. with something like findnext(array, start) you'd want to allow start being an integer or CartesianIndex.

base/abstractarray.jl Outdated Show resolved Hide resolved
base/abstractarray.jl Outdated Show resolved Hide resolved
@stevengj stevengj added domain:docs This change adds or pertains to documentation domain:arrays [a, r, r, a, y, s] labels Jan 2, 2024
base/multidimensional.jl Outdated Show resolved Hide resolved
Copy link
Sponsor Member

@inkydragon inkydragon left a comment

Choose a reason for hiding this comment

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

LGTM

@inkydragon inkydragon added the status:merge me PR is reviewed. Merge when all tests are passing label Jan 25, 2024
Copy link
Contributor

@barucden barucden left a comment

Choose a reason for hiding this comment

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

The argument type should not be restricted to AbstractArrays. We have similar methods for tuple arguments too. I left a few comments in that direction. The comments also apply for the documentation of nextind.

Please, add Fixes #42084 to the PR description.

@@ -209,7 +209,74 @@ String
"""
valtype(A::Type{<:AbstractArray}) = eltype(A)

"""
prevind(::AbstractArray, i)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
prevind(::AbstractArray, i)
prevind(A, i)

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Since there are prevind functions for other types of input, perhaps preserving the type signature is acceptable?

julia> methods(prevind)
# 8 methods for generic function "prevind" from Base:
 [1] prevind(s::AbstractString, i::Int64, n::Int64)
     @ strings\basic.jl:499
 [2] prevind(s::AbstractString, i::Integer, n::Integer)
     @ strings\basic.jl:495
 [3] prevind(s::AbstractString, i::Int64)
     @ strings\basic.jl:497
 [4] prevind(s::AbstractString, i::Integer)
     @ strings\basic.jl:496
 [5] prevind(a::AbstractArray{<:Any, N}, i::CartesianIndex{N}) where N
     @ Base.IteratorsMD multidimensional.jl:155
 [6] prevind(::AbstractArray, i::Integer)
     @ abstractarray.jl:212
 [7] prevind(t::Tuple, i::Integer)
     @ tuple.jl:77
 [8] prevind(t::NamedTuple, i::Integer)
     @ namedtuple.jl:180

Copy link
Contributor

Choose a reason for hiding this comment

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

Right now, we do not have any documentation for prevind(A, i) as reported in #42084. This PR provided documentation for the AbstractArray methods, but I meant to point out that, in order to fix the issue, this PR should introduce a single description that applies to any prevind(A, i) method no matter what A (and i) exactly are. In that sense, I would say the type signature should not be included.

We also have, e.g., the dot function documented as dot(x, y) (no type signatures) even though there are many two-argument methods of different argument types.

(...and the same for nextind)

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

JuliaHub's search results show that two-parameter prevind definition is more common.
Also, most prevind definitions are for String types.

Perhaps most people assume that for inputs of type Array, the spacing of the indexes is one by default, so there is no need to use a function to get the previous index.

This PR provided documentation for the AbstractArray methods, but I meant to point out that, in order to fix the issue

Yes, I think this pr only adds documentation to function with input type AbstractArray, and there are other types of input documentation indeed, so maybe it can't just be closed yet #42084 .

I have no preference for this at the moment, so please feel free to accept suggestions for changes or not. @JamesWrigley

Copy link
Contributor

Choose a reason for hiding this comment

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

Quickly skimmimg through the juliahub results, it seems that the methods still do the same thing: get the index that is right before a given index in some collection (maybe string).

We never provided any documentation so packages could implement the method in any way, I guess. But I believe we should be able to provide a generic description for the method. We were able to do that for firstindex, eachindex, etc.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

My idea is to let us add documentation for specific functions first.

If we want to recommend that people extend these functions (interfaces), in a uniform way, then the more recommended way would be to add another subsection to the corresponding manual that describes these functions and tells people how to implement them for custom data types.

More improvements could be made in follow up pr's.
I tend to make only a minimal amount of changes in each pr so that it's easy for people to review and merge quickly.

Copy link
Member

Choose a reason for hiding this comment

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

I think it is a good idea to document the most generic method before or even instead of documenting more specific methods. When documenting individual methods, we get more verbose documentation and still someone may still not know how to use this function in generic code where they do not know the collection type they are working with, nor how to implement the function on a custom type. From the manual

In general, only the most generic method should be documented, or even the function itself (i.e. the object created without any methods by function bar end). Specific methods should only be documented if their behaviour differs from the more generic ones. In any case, they should not repeat the information provided elsewhere.

I agree with @barucden that this PR's content (with minor modification) should be applied to the generic case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I made it generic in 648b68d.

base/abstractarray.jl Outdated Show resolved Hide resolved
base/abstractarray.jl Outdated Show resolved Hide resolved
base/abstractarray.jl Outdated Show resolved Hide resolved
inkydragon and others added 2 commits January 26, 2024 08:12
Co-authored-by: Denis Barucic <barucic.d@gmail.com>
Co-authored-by: Denis Barucic <barucic.d@gmail.com>
@JamesWrigley
Copy link
Contributor Author

(I assume you want to take over the PR @inkydragon so I'll let you respond to the other review comments)

@LilithHafner LilithHafner removed the status:merge me PR is reviewed. Merge when all tests are passing label Jan 27, 2024
@barucden
Copy link
Contributor

I think that all concerns have been addressed. In its current state, this PR closes #42084 in my opinion.

@LilithHafner LilithHafner changed the title Document the array methods for nextind() and prevind() Document the generic functions nextind() and prevind() Jan 28, 2024
Copy link
Member

@LilithHafner LilithHafner left a comment

Choose a reason for hiding this comment

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

I reverted the accidental removal of a comment and moved the docstrings from applying to the specific AbstractArray methods to applying to generic functions. To do this I had to move them to the base/tuple.jl file because that is where the first methods are defined.

@LilithHafner LilithHafner added the status:merge me PR is reviewed. Merge when all tests are passing label Jan 28, 2024
@LilithHafner LilithHafner merged commit 0ce62fe into JuliaLang:master Jan 28, 2024
6 of 8 checks passed
@LilithHafner LilithHafner removed the status:merge me PR is reviewed. Merge when all tests are passing label Jan 28, 2024
@JamesWrigley JamesWrigley deleted the nextind-prevind-docs branch January 28, 2024 22:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:arrays [a, r, r, a, y, s] domain:docs This change adds or pertains to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Documentation for prevind/nextind on tuples and arrays is missing
6 participants