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

Improve docstrings for Int, Float64, ^, and friends #45221

Merged
merged 37 commits into from
Feb 12, 2024

Conversation

mcabbott
Copy link
Contributor

@mcabbott mcabbott commented May 7, 2022

This extends the docs for Int, Float64 etc, to

  • Note that Int64 is the default, mention that it can overflow
  • Explain what 1f0 means, and that Float64 is the default

Similarly extends docs for ^, +, * aiming to

  • Point out that 1.2 * 10^3 is a bad habit, warn about overflow
  • Give an example for what literal_pow is doing since the explanation is quite technical
  • Also point out overflow in +, as suggested here
  • While there, mention that you can add vectors, and that vararg +(1,2,3,4) has a default binary behaviour
  • Similarly mention that vararg *(1,2,3,4) has a default order, from the left
  • While there, mention 1/2pi and v'v as examples of *.

base/promotion.jl Show resolved Hide resolved
Comment on lines 1783 to 1793
Real <: Number

Abstract supertype for all real numbers.

```jldoctest
julia> subtypes(Real)
4-element Vector{Any}:
AbstractFloat
AbstractIrrational
Integer
Rational
Copy link
Contributor Author

@mcabbott mcabbott May 7, 2022

Choose a reason for hiding this comment

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

This fails on CI, not sure why.

Help for subtypes has a similar example, is this one being processed earlier in loading or something?

(Now removed.)

base/docs/basedocs.jl Show resolved Hide resolved
@kshyatt kshyatt added the docs This change adds or pertains to documentation label May 7, 2022
base/docs/basedocs.jl Outdated Show resolved Hide resolved
base/docs/basedocs.jl Outdated Show resolved Hide resolved
base/docs/basedocs.jl Outdated Show resolved Hide resolved
Comment on lines 1829 to 1986
All unsigned integers are printed in hexadecimal, with prefix `0x`,
and can be entered in the same way.
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
All unsigned integers are printed in hexadecimal, with prefix `0x`,
and can be entered in the same way.
All unsigned integers are printed and can be parsed in hexadecimal, with prefix `0x`.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you say why?

I guess I basically want to say you can type that in, but type with a keyboard is unclear when there are data types around. But it's a minor point since by default most printing can be pasted back in. That anything you type in is parsed seems like something people who know know, and who don't, don't need to.

Copy link
Contributor

@halleysfifthinc halleysfifthinc May 10, 2022

Choose a reason for hiding this comment

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

Ah! I missed the potential ambiguity of type vs keyboard typing. "entered" reads awkwardly to me. Perhaps "written" instead?

Either

Suggested change
All unsigned integers are printed in hexadecimal, with prefix `0x`,
and can be entered in the same way.
All unsigned integers are printed in hexadecimal, with prefix `0x`,
and can be written in the same way.

or

Suggested change
All unsigned integers are printed in hexadecimal, with prefix `0x`,
and can be entered in the same way.
All unsigned integers are printed and can be written in hexadecimal, with prefix `0x`.

base/docs/basedocs.jl Outdated Show resolved Hide resolved
@mcabbott
Copy link
Contributor Author

Bump?

Related PRs (after this one) include #45975 (type trees for abstract types) #45415 (Int and UInt, in addition to Int64 etc.)

Comment on lines 2136 to 2142
"""
$UInt === UInt

$(Sys.WORD_SIZE)-bit unsigned integer type, `$UInt <: Unsigned <: Integer`.

Like [`Int`](@ref $Int), the alias `UInt` may point to either `UInt32` or `UInt64`,
according to the value of `Sys.WORD_SIZE` on a given computer.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As suggested by jakobnissen, this adds a note that UInt64 == UInt etc.

The docstring for Int64 has also had more detail added, to indicate how widely the default is used.

base/docs/basedocs.jl Show resolved Hide resolved

If `x` and `y` are integers, the result may overflow.
To enter numbers in scientific notation, use [`Float64`](@ref) literals
such as `1.2e3` rather than `1.2 * 10^3`.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@mcabbott mcabbott Oct 5, 2022

Choose a reason for hiding this comment

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

Not this PR, but I wonder whether Base.literal_pow(^, 10, Val(19)) ought to make a larger integer. I can picture wanting x^3 to return an Int and be fast, but if I write x^20 and supply x::Int might it be reasonable to guess that I don't want overflow? Maybe not...

Literal base + literal exponent would be safer, and could be argued should match long literal integers. But that's harder than adding a method.

Edit: now I saw #21600 which might be better.

Copy link
Member

Choose a reason for hiding this comment

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

For literals, if we don't change the current behavior, this seems like something a linter should catch, but that's not within the scope of the JuliaLang/julia repo.

@DilumAluthge DilumAluthge removed their request for review January 28, 2023 06:02
LilithHafner pushed a commit that referenced this pull request Jan 28, 2023
In particular:

* document the `order` keyword in `sort!`
* list explicitly the required properties of `lt` in `sort!`
* clarify the sequence of "by" transformations if both `by` and `order` are given
* show default values in the signatures for `searchsorted` and related functions
* add `isunordered` to the manual (it's already exported)
@mcabbott
Copy link
Contributor Author

Bump, maybe it's worth trying to finish this?

Today's question about 10^23 is: https://discourse.julialang.org/t/scientific-notation-vs-10/98849

@vtjnash
Copy link
Sponsor Member

vtjnash commented Nov 1, 2023

bump. Can we finish or close whatever is wanted here?

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.

This is almost ready to merge.

I left a few small comments, but the only merge-blocker is that, as is, this PR will make the web-hosted documentation state that Int64 is the type returned by length

Giving Int a docstring separate from Int64 and Int32 would be much better.

"""
   I32

32 bit integer
"""
struct I32 end

"""
   I64

64 bit integer
"""
struct I64 end

"""
    I

an alias for I32 or I64, depending on Sys.WORD_SIZE
"""
const I = Sys.WORD_SIZE == 64 ? I64 : I32
help?> I32
search: I32 Int32 Inf32 UInt32

  I32

  32 bit integer

help?> I64
search: I64 Int64 Inf64 UInt64 Int16 Inf16

  I64

  64 bit integer

help?> I
search: I IO if Int in im Inf I64 I32 isa Int8 inv Int64 Int32 Int16 imag Inf64 Inf32 Inf16 Int128 isqrt isone isodd isnan isinf isdir IdDict import invoke Integer iszero isreal

  I

  an alias for I32 or I64, depending on Sys.WORD_SIZE

base/docs/basedocs.jl Outdated Show resolved Hide resolved
base/docs/basedocs.jl Outdated Show resolved Hide resolved
"""
$Int

$(Sys.WORD_SIZE)-bit signed integer type, `$Int <: Signed <: Integer <: Real`.
Copy link
Member

Choose a reason for hiding this comment

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

This interpolation will make the web-hosted documentation falsely state that Int64 "is the type returned by functions such as length".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have a great solution here. Would welcome re-wording solutions if you feel it's necessary for the web version never to reflect the machine it was generated on. 32 bit systems are pretty rare, and if you keep reading it may make sense... can we just add a disclaimer?

@mcabbott
Copy link
Contributor Author

mcabbott commented Feb 11, 2024

Giving Int a docstring separate from Int64 and Int32 would be much better

It's been a while, but I'm pretty sure I tried this, and after a few days was unable to make CI / documenter understand it correctly. Perhaps it can be attempted in a follow-up PR.

@LilithHafner
Copy link
Member

It's been a while, but I'm pretty sure I tried this, and after a few days was unable to make CI / documenter understand it correctly.

If the code in #45221 (review) doesn't work that feels like a bug/shortcoming of Documenter, but I expect it should work. I'll try to push a commit using that approach.

Perhaps it can be attempted in a follow-up PR.

I don't want to merge something that knowingly introduces false statements to the documentation, especially this close to the feature freeze.

@LilithHafner
Copy link
Member

With 7e98348 we have

help?> Int64
search: Int64 UInt64 Inf64 Int8 Int16 Int32 Int UInt32

  Int64 <: Signed <: Integer

  64-bit signed integer type.

  Note that such integers overflow without warning,
  thus typemax(Int64) + Int64(1) < 0.

  See also Int, widen, BigInt.

help?> Int32
search: Int32 UInt32 Inf32 Int128 Int8 Int16 Int64 Int

  Int32 <: Signed <: Integer

  32-bit signed integer type.

  Note that such integers overflow without warning,
  thus typemax(Int32) + Int32(1) < 0.

  See also Int, widen, BigInt.

help?> Int
search: Int Int8 UInt Inf Int16 Int32 Int64 UInt8

  Int

  Sys.WORD_SIZE-bit signed integer type, Int <:
  Signed <: Integer <: Real.

  This is the default type of most integer literals,
  on computers for which Sys.WORDSIZE == 64, and has
  an alias Int. It is the type returned by functions
  such as length, and the standard type for indexing
  arrays. The default (and the alias) may be 32 or 64
  bits on different computers, indicated by
  Sys.WORD_SIZE.

  Note that integers overflow without warning, thus
  typemax(Int) + 1 < 0 and 10^19 < 0. Overflow can be
  avoided by using BigInt. Very large integer
  literals will use a wider type, for instance
  10_000_000_000_000_000_000 isa Int128.

  Integer division is div alias ÷, whereas / acting
  on integers returns Float64.

  See also Int64, widen, typemax, bitstring.

And all three docstrings are listed in the manual under en/base/numbers.html#Concrete-number-types. If Documenter.jl and CI are happy with this we can update the Int docstring appropriately.

@mcabbott
Copy link
Contributor Author

knowingly introduces false statements

This is a little strong! The suggestion was to water down the text sufficiently (which is easy) then merge. Then fight CI, if so inclined.

If you want to fight Documenter.jl and CI on this branch instead, that's also OK. It's entirely possible I missed some trick when I tried before.

@LilithHafner
Copy link
Member

Ah, I was assuming you were proposing merging as was.

Anyway, Documentation and CI are green after implementing my previous suggestion, no fight required.

base/docs/basedocs.jl Outdated Show resolved Hide resolved
base/docs/basedocs.jl Outdated Show resolved Hide resolved
base/docs/basedocs.jl Outdated Show resolved Hide resolved
base/math.jl Outdated Show resolved Hide resolved
@vtjnash vtjnash added the merge me PR is reviewed. Merge when all tests are passing label Feb 12, 2024
@LilithHafner LilithHafner merged commit 29a58d5 into JuliaLang:master Feb 12, 2024
8 checks passed
@mcabbott mcabbott deleted the doc6 branch February 12, 2024 14:13
@mcabbott
Copy link
Contributor Author

Thanks for pushing this over the finish line!

@LilithHafner
Copy link
Member

Thank you for doing the bulk of the work!

@giordano giordano removed the merge me PR is reviewed. Merge when all tests are passing label Feb 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs This change adds or pertains to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants