Closed domna closed 1 year ago
Oh we completely missed that case. Good find.
The args/kwargs issue aside, I think we should build an IndexDispersionSum, or something along those lines and allow the sellmeier dispersions for both, as it is only real...
About the how i have to think a little longer.
Yes that's a good line of thought. I wanted to bring an IndexDispersionSum
already while fixing #110 but gave up on the complexity, because I didn't want to screw it up with it. I'm also not sure if it would be better to distinct index dispersion and dielectric dispersion in the class hierarchy. That IndexDispersion/IndexDispersionSum is a child of Dispersion/DispersionSum adds some complexity to the adding behaviour and will further complex it when we have an IndexDispersionSum
. I have to think about how the class hierarchy would work nicely, too.
The thing with allowing Sellmeier for both is the problem in which representation we are thinking, as adding it to another dispersion is prohibited by having one dispersion in epsilon and one in n, not the dispersions itself being either in n or epsilon.
If we allow the model to be added together seamlessly we would open the door for unforeseen errors in the model construction. That's why I think a deliberate calling of as_index()
is a better way (when people lets say add a dispersion together from literature - like we do from rii).
I think it is also only physical if dispersions are added in eps representation (but I still have to think about that) and n representations are generally empirical and oldish.
I am completely with you.
The DispersionIndexSum
would complicate the whole system even more and generate even more isinstance
queries.
I think i grow to like the as_index()
, because of the explicitness.
Just for me, the only edgecases we have should be:
Maybe we should just handle them explicitly. Maybe even with a special Dispersion RII_Sellmeier_KTable
.
I don't have a full overview if there is only Sellmeier + k-table
, could also be another dispersion (i.e. cauchy, but this would be fine. This would, however, fail when we fix #119 because adding of index based dispersions is not yet properly handled).
n-table and k-table can also have different x-axis values, but we're fine since we are interpolating. Adding of tabular dispersions, however, is also a thing to solve properly 😬. My idea would be to only let pure real and pure imaginary overlap and otherwise use adding to extend the range? - or maybe we should explicitly handle range extension and don't solve it via adding to avoid confusion....
I don't have a full overview if there is only
Sellmeier + k-table
, could also be another dispersion (i.e. cauchy, but this would be fine. This would, however, fail when we fix #119 because adding of index based dispersions is not yet properly handled).
I think i prefer to handle them explicitly in the RII importer for now.
My idea would be to only let pure real and pure imaginary overlap and otherwise use adding to extend the range? - or maybe we should explicitly handle range extension and don't solve it via adding to avoid confusion....
Extending explicitly is better then using add for that case, as we will have to write functionality to choose averaging or selection of a or b if the ranges overlap.
I agree with all points. Let's do it like this.
Problem: I just realised that we use incompatible dispersions in the rii parser. There are epsilon-type dispersions added to n-type dispersions for tabulated k and formula n. This comes in due to the fact that rii operates in n-space and we convert it directly into epsilon. As I tried it out as I thought this should fail I realised that this case is not accounted for, which leads to problem #119. Applies only to tabulated k + formula n dispersions.
For example, using
rii.load_dispersion("glass", "BK7")
returns an incompatible and nestedDispersionSum
Solution(s):
DispersionSum
which should failDispersion
to return them as an index dispersion to allow using them as an index dispersion. Something likeas_index()
or so.