Minor fixes - review carefully

[jtravs]

No description provided.

[chrisbrahms]

Assuming tests pass I'm happy with this

[jtravs]

I get 25 failures (201 passes). So there is some bug hunting to do.

[jtravs]

But I also get 21 failures on current master. @chrisbrahms do you also get that?

[chrisbrahms]

But I also get 21 failures on current master. @chrisbrahms do you also get that?

Right, yes, should've reminded you. Most of the 21 failures are a consequence of #97 which I don't have a satisfactory solution to at the moment. Some others are because you updated Tools.jl but didn't update the tests--I fixed this on a branch somewhere but can't remember which one now.

[jtravs]

Can you point me to where I updated Tools.jl, I cannot find it. As far as I can see it should be fine.

[jtravs]

I think it wasn't a change I made, but the same problem with the dispersion. The value of β2 is totally wrong. I think it is the finite differences still.

[chrisbrahms]

In this commit, the field λz disappeared and you switched from using ω to using λ, but test_tools.jl did not change.

[jtravs]

OK, I fixed that.

Do you have a known commit when the Maths.derivative tests passed? I just went back to an early Feb commit and it did not work then. And then a 27th Nov commit and it didn't work then either. Did the Finite Differences ever pass these tests? If so I think we need to bisect this.

[chrisbrahms]

I think it's an issue/change with FiniteDifferences itself changing something, so you'd have to go back through versions of that instead. I'm sure it passed at some point--you must have set the rtol values in the test somehow, and we know the values we're comparing to are exact.

[jtravs]

FiniteDifferences v0.7.2 also doesn't work (released Sep 2019). I cannot install an older version due to compatibility.

[jtravs]

The version in the Manifest for the commit of these changes was 0.8.0. And that doesn't work. So I am now very confused.

[jtravs]

OK. On Julia 1.1.1, on my rectmodes branch using the exact package state (from the Manifest, incidentally this convinces me it is worth checking in the Manifests) everything passes. I will now start to Bisect to see when things went wrong.
BTW with Julia 1.3 it doesn't pass.

[jtravs]

So this current branch (and probably master as they're very close) pass all derivative tests, and also all capillary tests apart from one that seems unrelated, including all the dispersion, on Julia 1.1.1 and FiniteDifferences 0.8.0.

It also works fine with latest FiniteDifferences (and all dependencies at latest versions), on Julia 1.1.1 So something changed with Julia versions.

[jtravs]

See JuliaDiff/FiniteDifferences.jl#64

[chrisbrahms]

OK that's even weirder. I can't find anything in the release notes that would explain this.

[jtravs]

Those errors look like they'll be fixed by JuliaDiff/FiniteDifferences.jl#65

[chrisbrahms]

Given that the rest will be fixed externally, is there anything left to do on this?

[jtravs]

Yes, this broke some tests. I need to fix those first. Just finishing off Raman at the mo.

[jtravs]

I'd like to merge this very soon, as I foolishly based my Raman branch from it.
Apart from known test failures, this branch makes the gradient tests fail. I believe because you set the loss differently in the linear operator. I'll see if I can fix it.

[chrisbrahms]

I'd like to merge this very soon, as I foolishly based my Raman branch from it.
Apart from known test failures, this branch makes the gradient tests fail. I believe because you set the loss differently in the linear operator. I'll see if I can fix it.

How do the gradient tests fail--do they error? I'm actually just looking at the linear operator (make_const_linop) to finish off #99 because it's a mess

[jtravs]

field: Test Failed at C:\Users\John\Documents\GitHub\Luna\test\test_gradient.jl:59
  Expression: all((output_grad.data["Eω"])[2:end, :] .≈ (output_const.data["Eω"])[2:end, :])

it must be due to the change in handling of α because I didn't touch anything else.

[chrisbrahms]

It's actually setting the linop at ω=0 to 0 rather than 1. You did this here for the constant linop case but not for the gradient. I fixed this here so you can either do that here as well or just merge and it will be fixed when #99 is merged.