Open mikmoore opened 9 months ago
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!
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.
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.
It appears that some
Base
functions extended to::Quaternion
arguments contribute their own docstrings that are near-verbatim repeats of the existing versions fromBase
. 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 thatsign
works onRational
specifically, nor doesDualNumbers
feel the need to do the same forDual
. 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 thereal(::Quaternion)
methodconj
sign
-- we don't even provide a custom implementationinv
-- proposed by #133At least the
real
docstring adds literally something, if only references. I can't see whyquat
is relevant there, and it's already linked fromQuaternion
which seems a more reasonable place. Discoverability ofimag_part
is important (arguably, field access is fine) but I would argueimag
is a much better place to find it thanreal
. 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 liketo 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 theround(::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 ofBase.round
anyway, because no other version accepts 4RoundingMode
s. 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.