MLResearchAtOSRAM / tmm_fast

tmm_fast is a lightweight package to speed up optical planar multilayer thin-film device computation. Developed by Alexander Luce (@Nerrror) in cooperation with Heribert Wankerl (@HarryTheBird).
MIT License
53 stars 22 forks source link

Fix vectorized list_snell functions and is_not_forward_angle #24

Closed wet-dog closed 6 months ago

wet-dog commented 6 months ago

I believe this fixes #23.

Fix the vectorized list_snell functions to properly match the original tmm function

Correct is_not_forward_angle in vectorized_tmm_dispersive_multistack.py to match the version in tmm_fast_torch.py

The lines:

answer[torch.where(~(ncostheta.imag > 100 * EPSILON))] = ncostheta.real[torch.where(~(ncostheta.imag > 100 * EPSILON))] > 0
answer[torch.where(~(ncostheta.imag > 100 * EPSILON))] = ncostheta.real[torch.where(~(ncostheta.imag > 100 * EPSILON))] > 0

were missing the appropriate abs calls, so negative imag parts were not being flagged in the answer tensor. I decided to copy over the correct corresponding code in tmm_fast_torch.py instead of fixing this confusing use of torch.where.

Fix the typos that added ~'s.

wet-dog commented 6 months ago

Hi @Nerrror ,

My mistake :sweat_smile:, I haven't made a pull request before and I thought just my first commit would stay on the pull request and not all my following commits which I'm just doing for myself...

I'll fix that up.

Nerrror commented 6 months ago

Looks good, I'll go ahead with the merge

Nerrror commented 6 months ago

@wet-dog I think I found another problem with the previous torch implementation that is now in the main branch. I had implemented an incoherent version of the tmm_fast a while ago which was pushed only on develop. There, some error occured when some layers had absorption.

Could you try to insert

answer[torch.where(ncostheta.imag > 100 * EPSILON)] = ncostheta.imag[torch.where(ncostheta.imag > 100 * EPSILON)] > 0
answer[torch.where(~(ncostheta.imag > 100 * EPSILON))] = ncostheta.real[torch.where(~(ncostheta.imag > 100 * EPSILON))] > 0 

in the lines that you flag before and try to verify if your test example works ? they should only differ in the two ~ being removed in the first line

wet-dog commented 6 months ago

@Nerrror

Ah, you mean that me copying over the lines from the previous torch implementation might cause some error?

Inserting the lines you mention will cause an error with my example because those lines are still missing calls to abs.

If I understand what you're saying then using these lines works:

answer[torch.where(torch.abs(ncostheta.imag) > 100 * EPSILON)] = (
    ncostheta.imag[torch.where(torch.abs(ncostheta.imag) > 100 * EPSILON)] > 0
)
answer[torch.where(~(torch.abs(ncostheta.imag) > 100 * EPSILON))] = (
    ncostheta.real[torch.where(~(torch.abs(ncostheta.imag) > 100 * EPSILON))] > 0
)