Fix for wollam data without dpolE

[domna]

Fixes #213

[MarJMue]

I can't test it right now, but it looks correct.

[MarJMue]

I do not fully get what happened, but the code is definitely better to read and should be more robust.

[MarJMue]

Another note:
It is malicious behavior, but if you want to break stuff you can...
This obviously fails, so we should hint, that converted dispersions are fixed, even if they happily accept .add() methods

sell = Sellmeier()
sell.add(1, 1)

lbda = np.linspace(300, 900, 100)

assert_array_equal(
    sell.as_index().add(2, 2).get_dielectric(lbda),
    sell.add(2, 2).get_dielectric(lbda),
)

[domna]

There's still something generally wrong it seems

assert_array_equal(
    sell.add(2, 2).as_index().get_dielectric(lbda),
    sell.add(2, 2).get_dielectric(lbda),
)

AssertionError: 
Arrays are not equal

Mismatched elements: 100 / 100 (100%)
Max absolute difference among violations: 1.36134454
Max relative difference among violations: 22.68711656
 ACTUAL: array([ 5.287958e-01+0.j,  5.086200e-01+0.j,  4.879604e-01+0.j,
        4.668118e-01+0.j,  4.451687e-01+0.j,  4.230256e-01+0.j,
        4.003767e-01+0.j,  3.772161e-01+0.j,  3.535378e-01+0.j,...
 DESIRED: array([ 0.434555+0.j,  0.410344+0.j,  0.385552+0.j,  0.360174+0.j,
        0.334202+0.j,  0.307631+0.j,  0.280452+0.j,  0.252659+0.j,
        0.224245+0.j,  0.195203+0.j,  0.165523+0.j,  0.1352  +0.j,...

[MarJMue]

You added the oscillator a second time before the comparison. The add method changes the object permanently. It is working as intended.

Edit: Which might also be a problem in my example.

[domna]

Yes, this works:

def test_index_dispersion_conversion():
    sell = Sellmeier()
    sell.add(1, 1)

    lbda = np.linspace(300, 900, 100)

    assert_array_equal(
        sell.as_index().get_dielectric(lbda),
        sell.get_dielectric(lbda),
    )

    assert_array_equal(
        sell.as_index().add(2, 2).get_dielectric(lbda), sell.get_dielectric(lbda)
    )

[domna]

From a design perspective, it's a bit odd that we return the instance and alter it. Normally, it's either one of them (altering instance w/o instance return on function call or new instance return on function call)

[domna]

I'm dropping as_dielectric. The whole construct is just to cater the nk, k and n dispersion tables for rii which we cannot convert. We actually don't use as_dielectric anywhere and I think as_index should only be used in this particular case as this is very prone to produce errors if this is used wrongly.

[MarJMue]

From a design perspective, it's a bit odd that we return the instance and alter it. Normally, it's either one of them (altering instance w/o instance return on function call or new instance return on function call)

You are right. It is a bit confusing. I think altering it is the better way. If you want a second dispersion you tend to start from scratch anyway.

[domna]

yeah, so maybe we drop that we return the instance itself at some point. Then it feels more natural what's happening there. Anyways, out of scope for this PR :)

[MarJMue]

I'm dropping as_dielectric. The whole construct is just to cater the nk, k and n dispersion tables for rii which we cannot convert. We actually don't use as_dielectric anywhere and I think as_index should only be used in this particular case as this is very prone to produce errors if this is used wrongly.

Completely agree. Doesn't add any real value.

[domna]

I'll wait for confirmation from @MrJeremyFisher to see if it works for them, too, before I merge this in.

[MrJeremyFisher]

Sorry for the delayed response. These changes resolve my issue. Thanks for you work!

[domna]

Perfect! So I'll merge this in and it should be available with the next version.