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.

Sign up for GitHub

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

Jump to bottom

Some more compat annotations #30297

Merged
merged 2 commits into from
Dec 7, 2018
Merged

Some more compat annotations #30297

merged 2 commits into from
Dec 7, 2018

Conversation

fredrikekre
Copy link
Member

@fredrikekre fredrikekre commented Dec 7, 2018 •
edited
Loading

Compat annotation for unique!(f, A), #30141.
Compat annotation for rank(A; rtol=..., atol=...), #29926.

@fredrikekre
Compat annotation for unique!(f, A), #30141.
182409b 
Compat annotation for rank(A; rtol=..., atol=...), #29926.
@fredrikekre fredrikekre added the docs This change adds or pertains to documentation label Dec 7, 2018
@fredrikekre fredrikekre added this to the 1.1 milestone Dec 7, 2018
@fredrikekre fredrikekre changed the title Compat annotation for unique!(f, A), #30141. Some more compat annotations Dec 7, 2018
ararslan
ararslan approved these changes Dec 7, 2018
View reviewed changes
Copy link
Member

@ararslan ararslan left a comment

Choose a reason for hiding this comment

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

I don't know what the convention is, if any, for where to put the annotation within docstrings, but I would have voted for the end, after the examples. That said, you've done a bunch of these already, so I fully trust your judgement here. 😉

@fredrikekre
Copy link
Member Author

I don't know what the convention is, if any, for where to put the annotation within docstrings, but I would have voted for the end, after the examples.

Imagine how disappointed you would be after reading about an amazing function and even looked how useful it is from the examples, just to see that you have the wrong Julia version!

andreasnoack
andreasnoack reviewed Dec 7, 2018
View reviewed changes
stdlib/LinearAlgebra/src/generic.jl
@@ -713,7 +713,7 @@ end

"""
rank(A::AbstractMatrix; atol::Real=0, rtol::Real=atol>0 ? 0 : n*ϵ)
rank(A::AbstractMatrix, rtol::Real) = rank(A; rtol=rtol) # to be deprecated in Julia 2.0
rank(A::AbstractMatrix, rtol::Real)
Copy link
Member

Choose a reason for hiding this comment

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

Is this change intentional?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I included that information in the note, and we have not used this style anywhere else.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Agree, we should only document the methods and keywords that we actually want people to use. Any other supported-for-compatibility but no-longer-recommended methods/keywords should be in notes like this.

innerlee
innerlee reviewed Dec 7, 2018
View reviewed changes
stdlib/LinearAlgebra/src/generic.jl
!!! compat "Julia 1.1"
The `atol` and `rtol` keyword arguments requires at least Julia 1.1.
In Julia 1.0 `rtol` is available as a positional argument, but this
will be deprecated in Julia 2.0.
Copy link
Contributor

Choose a reason for hiding this comment

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

"also available"

Copy link
Member Author

Choose a reason for hiding this comment

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

No, on 1.0 it is not available as a kwarg.

@ararslan ararslan merged commit 4fc446f into master Dec 7, 2018
@ararslan ararslan deleted the fe/compat-annotations branch December 7, 2018 20:05
KristofferC pushed a commit that referenced this pull request Dec 7, 2018
@fredrikekre @KristofferC
Some more compat annotations (#30297)
797a24d 
* Compat annotation for unique!(f, A), #30141.
Compat annotation for rank(A; rtol=..., atol=...), #29926.

* Update stdlib/LinearAlgebra/src/generic.jl

(cherry picked from commit 4fc446f)
@KristofferC KristofferC mentioned this pull request Dec 7, 2018
Merged
52 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@StefanKarpinski StefanKarpinski StefanKarpinski left review comments

@andreasnoack andreasnoack andreasnoack left review comments

@innerlee innerlee innerlee left review comments

@ararslan ararslan ararslan approved these changes

Assignees
No one assigned
Labels
docs This change adds or pertains to documentation
Projects
None yet
Milestone
1.1
Development

Successfully merging this pull request may close these issues.
7 participants
@fredrikekre @StefanKarpinski @andreasnoack @ararslan @innerlee @JeffBezanson @KristofferC