Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix to deal with unphysical results in incoherent calculations for incident n > 1 #237

Merged
merged 7 commits into from Nov 30, 2022

Conversation

phoebe-p
Copy link
Member

In very specific circumstances (multi-layer incoherent/partially coherent stacks, at non-normal incidence, when the incident medium has n > 1) the inc_tmm function sometimes returns unphysical results. This happens when using the Fresnel equations with incidence/outgoing angles equal to or extremely close to pi/2. This situation only happens if you have total internal reflection and light could not reach that layer in any case, but there seems to be some numerical error because extremely large (>> 1) values for the transmission probabilities T and R are produced in these cases.

@codecov
Copy link

codecov bot commented Nov 11, 2022

Codecov Report

Merging #237 (bf03de5) into develop (3719ea9) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop     #237      +/-   ##
===========================================
+ Coverage    46.07%   46.11%   +0.04%     
===========================================
  Files           85       85              
  Lines         9133     9140       +7     
===========================================
+ Hits          4208     4215       +7     
  Misses        4925     4925              
Impacted Files Coverage Δ
solcore/absorption_calculator/tmm_core_vec.py 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Collaborator

@dalonsoa dalonsoa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it does maintain the condition, but is there any check that R + A + T == 1 when doing these manipulations?

About the underlying origin, is it just numerical accuracy issues or physical reason?

In any case, the changes look good to me.

tests/test_tmm_core_vec.py Outdated Show resolved Hide resolved
tests/test_tmm_core_vec.py Outdated Show resolved Hide resolved
@phoebe-p
Copy link
Member Author

Ok, so contrary to what I said last week, there actually is a check that R+A+T = 1 (in calculate_rat in transfer_matrix.py).
As for the origin of the problem, I think the issue is that when you have total internal reflection, obviously the transmitted intensity should be 0, but the transmitted angle becomes pi/2, so the path length is infinitely long, and this leads to numerical issues.

I found another issue in the calculation of incoherent profiles (this was due to a misunderstanding on my part; assuming Beer-Lambert absorption in incoherent layers is actually not quite correct, but there is no way to calculate the profile correctly without knowing the coherence length. I have kept the assumption of a Beer-Lambert-like absorption profile in these incoherent layers but now correctly scaled, and have added a note about this in the docstring of the relevant functions).

Finally, not related to these changes (the scheduled tests are also failing), the newest version of setuptools seems to break the installation. I don't know if this is related to #238? I have fixed it for now by specifying the previous version (setuptools==65.6.1) in the workflow files but that probably shouldn't be the long-term solution.

phoebe-p and others added 2 commits November 23, 2022 12:22
Co-authored-by: Diego Alonso Álvarez <6095790+dalonsoa@users.noreply.github.com>
Co-authored-by: Diego Alonso Álvarez <6095790+dalonsoa@users.noreply.github.com>
Copy link
Collaborator

@dalonsoa dalonsoa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great that you picked these issues! They are tricky, but have a large impact into the problem. I assume Rayflare have this sorted out by default, right?

@dalonsoa dalonsoa merged commit 47d1364 into develop Nov 30, 2022
@phoebe-p phoebe-p deleted the inc_tmm_fix branch February 13, 2023 05:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants