Add Jacobian

[vladimir-ch]

No description provided.

[vladimir-ch]

This is ready for review. PTAL @btracey

[btracey]

Maybe make it %0.4v or %0.6v? This level of precision might not be robust to different go editions.

[vladimir-ch]

Done with %0.6v.

[btracey]

maybe f := func(output, input)? The current code is clear for you and me, but for those who aren't aware of the convention

[vladimir-ch]

Changed to f := func(dst, x []float64) {, how about it?

[btracey]

Good.

[vladimir-ch]

I'm not sure if I like m being singled out in the signature but I see no other simpler way of communicating it to the function. I thought about binding it with f in a struct like

type VectorFunction struct {
    Func func(dst, x []float64)
    M    int
    N    int
}

and changing Jacobian to
func Jacobian(dst *mat64.Dense, f VectorFunction, x []float64, settings *JacobianSettings) *mat64.Dense
but perhaps it's not worth it. On the other hand the dimensions would be explicit and together with f.

[btracey]

The other option is to require that dst is non nil. This way the proper size information is contained in dst.Dims()

[vladimir-ch]

I was looking for something consistent with how Gradient behaves but yes, this would be a good option.

[vladimir-ch]

I'll leave it for another PR.

[btracey]

Okay.

[vladimir-ch]

PTAL @btracey

[btracey]

This looks correct, and I assume you tested with -race?

[btracey]

See above note.

[vladimir-ch]

I assume you tested with -race?

Now I have.

[vladimir-ch]

PTAL @btracey

[btracey]

Are you sure you tested with -race? This looks like a race condition to me. Multiple workerOrigin goroutines can be started, since making the origin non-nil does not happen before any part of the loop. Thus origin can be set several times, and it's not clear which will be the last.

This is easily fixed by changing the loop to if !hasOrigin, and then setting it below launching the goroutine.

[vladimir-ch]

Yes, I did test with -race but no test has a formula with multiple pt.Loc == 0. I silently assumed that locations (here only origin matters) in formulas are unique, although we do not check it, enforce it, or say nothing in respect in the docs. The formula d^k f(x) ≈ ... in the comment for Formula does not rule out repeated locations even with different coefficients, it just leads to unnecessary function evaluations.

I made a change that avoids (hopefully, please check) the racy behaviour and always uses the last occurrence of origin location in a Formula but it doesn't feel satisfactory, the rest of the logic still assumes that origin is not repeated. The simplest thing to do would be prohibit repeated origin locations in formulas completely.

[btracey]

LGTM

I agree we should check about repeated locations. In the meantime, good to be defensive about race conditions.