Docstring pollution

[mikmoore]

It appears that some Base functions extended to ::Quaternion arguments contribute their own docstrings that are near-verbatim repeats of the existing versions from Base. They don't provide any new information but they are printed via >help? anyway, cluttering the output. It doesn't seem sustainable for every package that adds a new type to document semantically-unchanged functions just for the sake of saying that they do exactly what the docstrings already said they do. If anything, this makes them more confusing (in addition to the clutter) because the user may try to spot the difference.

Other packages don't seem to take this approach. Base doesn't feel the need to document that sign works on Rational specifically, nor does DualNumbers feel the need to do the same for Dual. For that matter, Quaternions doesn't (yet) feel the need to do this for the majority of its extended functions (arithmetic, ==, exp, sin, etc.).

Offending functions at the present time appear to be:

• real -- only adds a "See also: imag_part, quat" to the real(::Quaternion) method
• conj
• sign -- we don't even provide a custom implementation
• inv -- proposed by Update around inv #133

At least the real docstring adds literally something, if only references. I can't see why quat is relevant there, and it's already linked from Quaternion which seems a more reasonable place. Discoverability of imag_part is important (arguably, field access is fine) but I would argue imag is a much better place to find it than real. That said, I wouldn't be excited to clutter that docstring either. I despise "not implemented" errors, but this is more of a "the operation is ill-defined for this type" error so I could be convinced to do something like

Base.imag(::Quaternion) = throw(ArgumentError("`Base.imag(::Quaternion)` is not defined. See Quaternions.imag_part`."))

to help discoverability. I vaguely recall there were "error hints" being considered for Base at some point (which seems like an even better solution), but I don't know if those ever became a thing.

It's unfortunate that round does appear to need ::Quaternion-specific documentation for the imaginary-part rounding modes, but I don't see a way around that. The only thing I would have considered (breaking change now, technically, although perhaps not in any actual use) would be that the imaginary parts all share a rounding mode (in which case we would have followed the round(::Complex,...) semantics). It's not easy to imagine that many people have needs to round imaginary parts differently from each other, so I wouldn't have hated making users roll-their-own for that level of control. It doesn't benefit from being part of Base.round anyway, because no other version accepts 4 RoundingModes. But this is a matter for a separate issue and not one that I feel strongly about since it'd be breaking to remove that functionality.

[hyrodium]

In my opinion, having these docstrings is great to provide examples with jldoctest.
I hope REPL's help-mode to be more interactive so that users can choose which of the docstrings to show.

I can't see why quat is relevant there

Because the original docstring refers to complex.

help?> real
search: real Real realpath isreal readlink readline readlines ReadOnlyMemoryError reenable_sigint isreadonly isreadable @threadcall readuntil readavailable ReentrantLock ProcessFailedException replacefield! read read! readdir readeach readchomp readbytes! break isready foreach Threads IOStream extrema!

  real(z)

  Return the real part of the complex number z.

  See also: imag, reim, complex, isreal, Real.

I despise "not implemented" errors, but this is more of a "the operation is ill-defined for this type" error so I could be convinced to do something like [...] to help discoverability.

That seems acceptable. PRs are welcomed!

[sethaxen]

Several of these are added because the version of the docstring in Base julia explicitly mentions function behavior for complex numbers, which does not describe behavior for quaternions. So not including the docstring is incomplete. e.g.

• Base.real: Return the real part of the complex number z.
• Base.conj: Compute the complex conjugate of a complex number z.

In these cases Base julia seems to be assuming the number is something like hypercomplex, so more general than what is documented.

For sign, the docstring in Base is sufficient. We include this docstring primarily so it is clear that this is how one should construct a unit quaternion (as opposed to the normalize we previously had and which might be more intuitive). Since many numerical uses of quaternions are the unit case, I feel this is important to document.

inv is a case where the docstring in Base is clearly sufficient, and we could certainly remove it.

I despise "not implemented" errors, but this is more of a "the operation is ill-defined for this type" error so I could be convinced to do something like

Base.imag(::Quaternion) = throw(ArgumentError("`Base.imag(::Quaternion)` is not defined. See Quaternions.imag_part`."))

I agree raising a "not implemented" error is not great. This is indeed a perfect case for error hints with Base.Experimental.register_error_hint. While they are experimental, they are widely used, e.g. https://github.com/SciML/SciMLBase.jl/blob/v2.33.1/src/debug.jl. I'd certainly welcome this in a PR.

[mikmoore]

A docstring does not help discoverability. Once one is reading help?> conj or help?> sign, one has already found the function they want and are merely asking about usage. One doesn't reach for conj without understanding what it is and why they want it. Discovery is the job of package docs. In that light, it seems that the primary purpose of these in-code docstrings is to have them to be included in the package docs. But we can simply move the redundant docstrings to the package docs directly, if we want them there. Or, at the very least, we could hide them under an extended help docstring.

I understand that these docstrings are not literally 100% redundant if one draws a strong distinction between "complex" and "hyper-complex", but note that the existing docstrings refer to "complex number" instead of "Complex <: Number". For that matter, these functions also work for real numbers despite the fact that this is not described (although might be inferable as real is a subset of complex, but that same intuition could extend these to hypercomplex). This is why the minor distinction does not bother me enough that I support an added docstring block.

The real(T::Type{<:Quaternion}) docstring literally describes nothing that real(::Type) does not. It merely provides an additional example. And jldoctest is no substitute for normal tests (which we have), nor is the behavior of any of these functions un-intuitive next to the existing docstrings.

I honestly cannot remember if it was always this way, but LinearAlgebra.normalize works for Number at the present. One could argue that the normalize behavior of returning NaN on zero input is useful in some situations as those NaN are more likely to propagate than the zeros given by sign. So I'll repeat my particularly strong objection to adding a docstring to sign because we literally provide no new code (nor is there interesting emergent behavior) and there exists a compelling competing use of normalize.

If every package added docstrings to every function that experienced a trivial epsilon extension, any medium or large project would risk having functions with unreadably-long docstrings. Docstrings are useful because they are short. That's why I object to many of these. I would not object to these same descriptions exclusively within the package docs.