ReactiveBayes / ReactiveMP.jl

High-performance reactive message-passing based Bayesian inference engine
MIT License
103 stars 14 forks source link

Docs should contain list of exported methods #114

Closed wmkouw closed 2 years ago

wmkouw commented 2 years ago

Feedback from users:

It is bad practice (common to many programming languages) to import all methods implicitly, e.g., from numpy import *, as it might silently overwrite a reference. So, users would like to explicitly import ReactiveMP: update!, ... instead of using ReactiveMP.

However, the documentation does not list all methods that ReactiveMP exports. Adding the remainder would be helpful.

wmkouw commented 2 years ago

Note that implicit imports are less dangerous due to Julia's multiple dispatch feature.

bauglir commented 2 years ago

So, users would like to explicitly import ReactiveMP: update!, ... instead of using ReactiveMP.

Note that if you don't intend to add methods to functions that you import you should probably use using instead, i.e. using ReactiveMP: update! works just as well. See this section and the following ones of the manual.

It is bad practice

I'm not sure that's actually considered bad practice in Julia, i.e. this is being done almost everywhere. Especially in more recent Julia versions this should not be an issue (you can even rename stuff you import/using nowadays). Personally I prefer being explicit as well, but I don't think it's considered bad practice not doing it.

it might silently overwrite a reference.

Julia doesn't do this (and it doesn't have to do with multiple dispatch). Imagine two packages Foo and Bar that both export the same struct Quuz. Julia will recognize that the reference to Quuz is ambiguous and warn you that you'll need to use the fully qualified name. For instance, on 1.7.1:

julia> using Foo
julia> using Bar
julia> Quuz
WARNING: both Bar and Foo export "Quuz"; uses of it in module Main must be qualified
ERROR: UndefVarError: Quuz not defined

or in a fresh session

julia> struct Quuz
         x
       end
julia> using Foo
WARNING: using Foo.Quuz in module Main conflicts with an existing identifier.
julia> using Bar
WARNING: using Bar.Quuz in module Main conflicts with an existing identifier.
julia> Quuz(1)
Quuz(1)
julia> Foo.Quuz()
Foo.Quuz()

All of that being said, making sure all docstrings are included in the manual is definitely a good thing. Note that Documenter is already telling you that quite a few docstrings aren't included. My personal approach to this is to only apply docstrings to things I actually want documented in either Public or Private API pages and use simple comments for everything else where I'm likely to be looking at the code anyway.

bvdmitri commented 2 years ago

Thanks @bauglir for such a nice explanation. I will add a bit of my own thoughts here. Generally it is good to have something like this in our documentation, I will keep this issue open until I add this. However, Julia is a different language and has different best practices. As far as I understand, import only a small number of functions from package is actuallty a bad practice. Users should generally do using SomePackage and import everything and package marked with export keyword. I find it quite convenient BTW, especially after experience in Java where you have 100+ lines of import statements, in Julia you just do using Package1, Package2, Package3 and language resolves everything for you automatically.

This might be counterintuitive for users from python, but Julia is a different language and has different best practices. We should add a list of exported methods in our documentation, but we can also be explicit about that and add a small note that using SomePackage is super safe in Julia.

as it might silently overwrite a reference

That will never happen in Julia. Julia merges same names, that actually what you want from multiple dispatch. We have Distributions.mean(::Normal) and ReactiveMP.mean(::PointMass), but these are both the same mean function. Julia does not overwrite one function with another one. If there are some conflicts Julia cannot resolve automatically then it throws a warning and usage of such a function is disallowed without explicit module resolution.

Explicit import might be even a bit more confusing, e.g.:

# Works perfectly in Julia, but makes little sense in other languages
import Distributions: mean
import ReactiveMP: mean
# I would prefer instead 
using Distributions, ReactiveMP

However, the documentation does not list all methods that ReactiveMP exports.

There is built-in Julia function for that: names(ReactiveMP)

wmkouw commented 2 years ago

Thanks for the clarifications guys

bauglir commented 2 years ago

but these are both the same mean function

Just for completeness' sake, there's an important caveat here. This is only true because you explicitly extend the mean function from Distributions and import mean from Distributions into ReactiveMP's scope.

Consider the difference between the behavior of the following Foo and Bar module

julia> module Foo
         import Distributions
         Distributions.mean(::String) = "This is now a thing"
       end
Main.Foo

julia> Foo.mean
ERROR: UndefVarError: mean not defined
Stacktrace:
 [1] getproperty(x::Module, f::Symbol)
   @ Base ./Base.jl:35
 [2] top-level scope
   @ REPL[3]:1

julia> mean
ERROR: UndefVarError: mean not defined

julia> using Distributions

julia> mean("Foo")
"This is now a thing"

julia> module Bar
         import Distributions: mean
         mean(::String, ::String) = "This is now also a thing"
       end
Main.Bar

julia> mean("foo", "bar")
"This is now also a thing"

julia> Bar.mean("foo", "bar")
"This is now also a thing"

julia> Bar.mean("foo")
"This is now a thing"

There is no magic that makes this universally true, i.e. just because two functions in two disparate modules have the same name, does not make them the same function. For instance

ulia> module Foo
         function same() end
      end
Main.Foo

julia> module Bar
         Main.Foo.same(::Int64) = 4
         notsame(::Int64) = 123
       end
Main.Bar

julia> module Baz
         Main.Foo.same(::Float64) = 5.0
         notsame(::Float64) = 123.0
       end
Main.Baz

julia> methods(Foo.same)
# 3 methods for generic function "same":
[1] same() in Main.Foo at REPL[1]:2
[2] same(::Int64) in Main.Bar at REPL[2]:2
[3] same(::Float64) in Main.Baz at REPL[3]:2

julia> methods(Bar.notsame)
# 1 method for generic function "notsame":
[1] notsame(::Int64) in Main.Bar at REPL[2]:3

julia> methods(Baz.notsame)
# 1 method for generic function "notsame":
[1] notsame(::Float64) in Main.Baz at REPL[3]:3
bvdmitri commented 2 years ago

You are right @bauglir , I skipped some of these details for the sake of my example, e.g. that it might be even more confusing to import SomePackage: mean twice from different packages but it might still refer to the same function.

My opinion in general is that Julia users should just do using SomePackage, but ofc I understand some confusion from python users. If there are some unresolvable conflicts, which does not happen often, Julia will complain anyway and will drag users attention to some conflicting names.

KeithWM commented 2 years ago

Hi all,

I know the PR has been merged, and I think agree that the best practices in Julia are different, and possibly rightfully so. But I do think it's still worth pointing out that even if importing all elements from a package individually might be unfavourable in "real code", it can be useful to do so in demos and examples. As a fairly strict "user" and not a developer of ReactiveMP, I do not always find it clear if functions, macros and structs in the examples come from the ReactiveMP package or from elsewhere. With the curse of knowledge this might all be blatantly obvious, but when approaching the package anew, this question can really get in the way of understanding what is going on.

bvdmitri commented 2 years ago

@KeithWM It is an interesting point I did not think about before. I'm not sure how does it work with "re-exported" symbols, e.g. ReactiveMP reexports dot from LinearAlgebra package (in the same way as we reexport mean from Distributions and Distributions by itself reexports mean from Statistics).

Technically speaking import ReactiveMP: dot does not necessarily mean that dot is defined within ReactiveMP (it is not). TBH I'm also not always entirely sure what function comes from what package, but IMO it is not a big problem in Julia. Usually it is sufficient to just call ?dot either in REPL or in jupyter cell to quickly check documentation for a specific symbol from all packages. But this approach will not help if you only read code without executing it...

bauglir commented 2 years ago

The @which macro might also come in useful in scenarios where you want to find out which exact method is being called.

For instance, @which 1 + 1 +(x::T, y::T) where T<:Union{Int128, Int16, Int32, Int64, Int8, UInt128, UInt16, UInt32, UInt64, UInt8} in Base at int.jl:87