Conflict in `Base.rand`

[JoseKling]

The method rand(pp<:AbstractPointProcess, tmin, tmax) conflicts with the method rand(X, d::Integer, dim::Integer) from Random.jl when tmin and tmax are integers.

Any way around this? Or just change the method name to simulate?

[jucheval]

What if you define a method rand(::Type{<:AbstractPointProcess}, tmin::Integer, tmax::Integer) which converts tmin and tmax to Float's ? I guess that this new method should get priority over the one from Random.jl, no ?

[gdalle]

As explained in #57, forcing float types is not necessarily a good idea.

The reason for this conflict is me abusing the Random API by overloading rand instead of properly defining a sampler, as explained here: https://docs.julialang.org/en/v1/stdlib/Random/#rand-api-hook. I think that is the correct solution.

[rsenne]

"""
    PointProcessSampler(pp, tmin, tmax)

Sampler for a point process `pp` over the time interval `[tmin, tmax)`.
"""
@kwdef struct PointProcessSampler{PP<:AbstractPointProcess, T<:Real} <: Random.Sampler
    pp::PP
    tmin::T
    tmax::T
end

function Random.Sampler(rng::AbstractRNG, pp::AbstractPointProcess, tmin::T, tmax::T) where T<:Real
    return PointProcessSampler(pp, tmin, tmax)
end

function Base.rand(rng::AbstractRNG, s::PointProcessSampler)
    return simulate_ogata(rng, s.pp, s.tmin, s.tmax)
end

function Base.rand(pp::AbstractPointProcess, tmin::T, tmax::T) where T<:Real
    return rand(Random.default_rng(), pp, tmin, tmax)
end

Is this the basic idea of what we are looking for?

[JoseKling]

I tested this and apparently Random.Sampler is a concrete type, so PointProcessSampler cannot be a subtype. You can even instantiate it with, for example,

Random.Sampler(Random.default_rng(), MultivariatePoissonProcess)

I had a quick look at the the link provided by @gdalle, but I don't really see how to implement this or what the benefit is.

One question is, how would this solve the original issue?

hp = HawkesProcess(1, 2, 3)
simulation = rand(hp, 0, 100)

ERROR: MethodError: rand(::HawkesProcess{Int64}, ::Int64, ::Int64) is ambiguous.

If there is no change in how we call the rand method, wouldn't this persist?

I think using simulate instead of rand as the method's name might make more sense. I would think we sample arrival times, but we simulate processes.

[JoseKling]

Hi guys,

any other ideas here?

[gdalle]

I tried diving a bit deeper into the Random docs but I'm not sure I fully understand the Sampler API, especially with an additional argument like the length of the time interval on which to sample. I think replacing the name is reasonable as a first step

[rsenne]

Yeah i think this is a good approach. I can't say I've been able to figure it out either. Maybe we can open an issue to the devs of Random and ask them about our use case?

[JoseKling]

Sure, that's a good idea.

But I was having another look, and in the documentation it says that in order to subtype Sampler, you need to provide the type of the output, for example,

"""
    PointProcessSampler(pp, tmin, tmax)

Sampler for a point process `pp` over the time interval `[tmin, tmax)`.
"""
@kwdef struct PointProcessSampler{PP<:AbstractPointProcess, T<:Real} <: Random.Sampler{Vector{Float64}} 
    pp::PP
    tmin::T
    tmax::T
end

this does not cause an error. Basically, there is an abstract type Sampler{T}, but Sampler is just a method for overloading. I guess the solution is really something on the lines of what @rsenne proposed, although I am still not sure I completely understand the API.

[JoseKling]

So, @rsenne. I guess we can just change the method's name to simulate for now and later on, if we want/need, we can always revert back with a proper implementation. This way we can focus on actually building some functionality. What do you say?

[rsenne]

I think that sounds like a good plan to me. I say we do that