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 low temperature problem of Van Regemorter approximation #1992

Merged
merged 4 commits into from May 10, 2022

Conversation

chvogl
Copy link
Contributor

@chvogl chvogl commented Apr 29, 2022

The exponential in the calculation of the van Regemorter approximation can become infinite, e.g. for 100 K and transition energies bigger than roughly 6.5 eV:
np.exp(((6.5 * units.eV) / (100 * units.K * const.k_B)).to(''))
The product of the exponential integral E1 and the exponential, however, stays finite. I have added an implementation of
the product that does not calculate the exponential for large values but instead uses the Laurent series expansion.

How has this been tested?

  • Testing pipeline.
  • Other.

Type of change

  • Bug fix.
  • New feature.
  • Breaking change.
  • None of the above.

Checklist

  • My change requires a change to the documentation.
    • I have updated the documentation accordingly.
    • (optional) I have built the documentation on my fork following the instructions.
  • I have assigned and requested two reviewers for this pull request.

@chvogl chvogl requested review from Rodot- and epassaro April 29, 2022 14:17
@codecov
Copy link

codecov bot commented Apr 29, 2022

Codecov Report

Merging #1992 (91856bb) into master (ae19f26) will increase coverage by 0.04%.
The diff coverage is 92.85%.

❗ Current head 91856bb differs from pull request most recent head e048c6c. Consider uploading reports for the commit e048c6c to get more accurate results

@@            Coverage Diff             @@
##           master    #1992      +/-   ##
==========================================
+ Coverage   59.99%   60.04%   +0.04%     
==========================================
  Files          70       70              
  Lines        8111     8121      +10     
==========================================
+ Hits         4866     4876      +10     
  Misses       3245     3245              
Impacted Files Coverage Δ
tardis/plasma/properties/atomic.py 56.17% <88.88%> (+1.07%) ⬆️
tardis/io/atom_data/base.py 89.71% <100.00%> (+0.17%) ⬆️
tardis/plasma/standard_plasmas.py 74.48% <100.00%> (+0.26%) ⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

Copy link
Contributor

@Rodot- Rodot- left a comment

Choose a reason for hiding this comment

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

Please provide new tests for these class methods to verify and guarantee their behavior. The test should verify that 500 is a good upper limit for the scipy/numpy calculation.

@chvogl chvogl force-pushed the fix-van-regemorter-low-T branch from e019c81 to 0368afa Compare May 9, 2022 21:30
@chvogl chvogl requested a review from Rodot- May 9, 2022 21:52
@Rodot- Rodot- merged commit c188b8a into tardis-sn:master May 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants