Minor issues with `History`

[JoseKling]

1 - Field types

Right now, h = History([0.5], ["a"], 0, 1) does not work, since the same type must be used in times, tmin and tmax.
Any reason to not make times, tmin and tmax all take Float64 and let the arguments be automatically converted?

2 - Add checks?

The constructor should probably check if all the events are inside the interval [tmin, tmax], it times is sorted and if times and marks have the same length.

3 - Non-marked history

Probably makes sense to have a separate constructor for non-marked history History(times, tmin, tmax). Methods like event_marks and max_marks should have separate implementations, maybe just returning nothing?

[jucheval]

Here are some remarks on your 3 items :
1/ What about adding a method like History(times::Vector{T1}, marks::Vector{M}, tmin::T2, tmax::T3) which creates a History{M,T} where T = promote_type(T1, T2, T3) ?
2/ I agree and I don't see other checks that should be added.
3/ Good idea

[JoseKling]

1 - I am struggling to see why not to fix the types of times, tmin and tmax to Float64. That's how I see it

1. The parametrization makes the code more complex, but with no benfit.
2. In the end, whenever we need to simulate, estimate or do almost anything these values will most probably be converted to Float64 anyway in the calculations.
3. Why should a History with times = [1, 2, 3], tmin = 0 and tmax = 4 be any different than a History with times = [1.0, 2.0, 3.0], tmin = 0.0 and tmax = 4.0? Or even than times = [1, 2, 3], tmin = 0.0, tmax = 4.0? For the marks, in contrast, this distinction makes sense.
   Or am I missing something?

2 - Not very important, but I just thought that, in theory, since History is mutable, any invariant we force at creation cannot be guaranteed afterwards. For example, even with the checks, it would be possible to do

h = History([1, 2, 3], ["a", "b", "c"], 0, 4)
h.tmax = -3

In practice, I am not sure how much of a problem this would be, but we could simply make it non-mutable. Right now it seems that only simulate_ogata uses push!, but this could be easily modified.

[jucheval]

Regarding item 1/, there are some subtypes of ‘Real’ which are used by auto diff packages. I guess that it would be weird to differentiate wrt a time of an History. However automatic differentiation may be interesting for parameters of a model (like the Hawkes you added)
Surely @gdalle has a more expert opinion on this subject.

[gdalle]

I'll answer tonight in more detail but type genericity is very important in Julia programming, and I'll try to explain why later! Julien has one answer but there are many others

[JoseKling]

Thanks @jucheval, makes sense.

Also, I guess it would just integrate better with other packages or uses that might use other types, right? Although I would think AbstractFloat makes more sense than Real?

Anyway, look forward to hear from @gdalle too.

[gdalle]

Right now, h = History([0.5], ["a"], 0, 1) does not work, since the same type must be used in times, tmin and tmax.
Any reason to not make times, tmin and tmax all take Float64 and let the arguments be automatically converted?

The parametrization makes the code more complex, but with no benfit.

In the end, whenever we need to simulate, estimate or do almost anything these values will most probably be converted to Float64 anyway in the calculations.

Programming with generic types is a very very common paradigm in Julia, because it enables composability between your package and others. You can never know what other people will want to use your package for, and if you restrict the types too much in advance, you may never find out. For instance, people may want to use the following number types when calling functions from PointProcesses.jl:

• floating point numbers with variable precision like Float32
• arbotrary precision numbers BigFloat
• dual numbers from ForwardDiff.jl
• numbers with error bars from Measurements.jl
• numbers with intervals from IntervalArithmetic.jl
• numbers with physical units from Unitful.jl
• numbers on a log scale from LogarithmicNumbers.jl
• the list goes on

A classical exposition of this principle is The Unreasonable Effectiveness of Multiple Dispatch, which I encourage you to watch. I also try to make this case in chapters 3 and 4 of my PhD dissertation.

You're right that this constructor failing is a bit tedious, but it's rarely a problem in real applications since there is no reason for the end time to be an integer for instance. Still, the right way to fix it is adding type conversion and promotion to the inner History constructor, like Julien suggested:

mutable struct History{M,T<:Real}
    times::Vector{T}
    marks::Vector{M}
    tmin::T
    tmax::T
    function History(times::Vector{T1}, marks::Vector{M}, tmin::T2, tmax::T3) where {T1,T2,T3,M}
        T = promote_type(T1, T2, T3)
        times_righttype = convert(Vector{T}, times)
        tmin_righttype = convert(T, tmin)
        tmax_righttype = convert(T, tmax)
        return new{M,T}(times_righttype, marks, tmin, tmax)
    end
end

julia> h = History([0.5], ["a"], 0, 1)
History{String, Float64}([0.5], ["a"], 0.0, 1.0)

Why should a History with times = [1, 2, 3], tmin = 0 and tmax = 4 be any different than a History with times = [1.0, 2.0, 3.0], tmin = 0.0 and tmax = 4.0? Or even than times = [1, 2, 3], tmin = 0.0, tmax = 4.0? For the marks, in contrast, this distinction makes sense.
Or am I missing something?

It's not different in terms of how a human looks at it. But in terms of the code that gets executed, it calls different methods due to the different number types. It may also occupy a different volume in memory (twice smaller for Float32, for instance).

Also, I guess it would just integrate better with other packages or uses that might use other types, right? Although I would think AbstractFloat makes more sense than Real?

Some of the types listed above do not subtype AbstractFloat, only Real. This is the case for ForwardDiff.Dual numbers, among others.
A floating point number is not a mathematical concept, it's an implementation detail, which is not relevant to the algorithms you're writing here.

The constructor should probably check if all the events are inside the interval [tmin, tmax], it times is sorted and if times and marks have the same length.

This makes sense when the History is constructed manually, or read from an external source. However, when it comes directly from the package's routines, that may be a slight waste of time, since you are supposed to guarantee these properties yourself. I would add the checks first, and then if they turn out to be a bother in real-life applications, perhaps make them optional via a keyword argument to the constructor.

Probably makes sense to have a separate constructor for non-marked history History(times, tmin, tmax). Methods like event_marks and max_marks should have separate implementations, maybe just returning nothing?

Instead of an additional constructor, you could also reorder the arguments to have an optional one at the end, like:

History(times, tmin, tmax, marks=fill(nothing, length(times)))

Note that carrying around this Vector{Nothing} is basically free, Julia is smart enough to not allocate:

julia> @allocated Vector{Nothing}(undef, 10)
0

julia> @allocated Vector{Nothing}(undef, 10^12)
0

Furthermore, the result of event_marks is correct. As for max_marks, as I remember it was mostly useful to get the range of possible marks in a categorical point process, so perhaps it should not be a method for general History types (or at least not a part of the public API).

Not very important, but I just thought that, in theory, since History is mutable, any invariant we force at creation cannot be guaranteed afterwards. For example, even with the checks, it would be possible to do

That is a bummer, you're right, and I'm not very satisfied with that part of the design either. I think I did that to enable iterative extension inside the Ogata simulation algorithm, but I'd favor a move to an immutable type. You could then modify Ogata's algorithm to work with the components times, marks, tmin, tmax and re-create a History every time you pass them to the conditional intensity function.

[JoseKling]

Thanks for the answer, @gdalle! I guess History{M,T<:Real} makes sense. Although apparently the types defined in Unitful.jl are <: Number, but not <: Real.

[gdalle]

The distinction between Number and Real honestly doesn't matter much here, it's a coin toss which one we choose. Real makes more mathematical sense, Number allows slightly more general types (but Unitful brings its own set of issues and would probably require individual field types T1, T2, T3 instead of a common one T, so let's leave it aside)

[JoseKling]

Solved in PR #61