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

LU decomposition for NLTE population solver #2465

Closed
wants to merge 33 commits into from

Conversation

AlexHls
Copy link
Member

@AlexHls AlexHls commented Nov 2, 2023

📝 Description

Type: 🚀 feature
Attempt to implement a LU decomposition solver for the NLTE population equation.

Currently as extra module, but if it works it should replace the root find method (probably).

Copy link

codecov bot commented Nov 2, 2023

Codecov Report

Attention: 35 lines in your changes are missing coverage. Please review.

Comparison is base (a7534c1) 67.17% compared to head (c3bd2b6) 68.67%.
Report is 2 commits behind head on master.

Files Patch % Lines
tardis/plasma/tests/test_nlte_solver.py 62.26% 20 Missing ⚠️
...dis/plasma/properties/nlte_rate_equation_solver.py 94.57% 12 Missing ⚠️
tardis/plasma/standard_plasmas.py 66.66% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2465      +/-   ##
==========================================
+ Coverage   67.17%   68.67%   +1.50%     
==========================================
  Files         156      157       +1     
  Lines       13763    13978     +215     
==========================================
+ Hits         9245     9600     +355     
+ Misses       4518     4378     -140     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@sonachitchyan sonachitchyan left a comment

Choose a reason for hiding this comment

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

Needs rebasing

@AlexHls AlexHls requested a review from andrewfullard November 9, 2023 19:49
@AlexHls
Copy link
Member Author

AlexHls commented Nov 9, 2023

Any comments on the new structure? I've merged both solvers into one file, sharing function calls wherever possible. There is still a bit more to be done (docs, tests etc.), but I thought it's probably best to get some feedback at this point before I start polishing this a bit more...
(This should be, if I didn't miss anything, a non-breaking feature)

@AlexHls AlexHls requested a review from sonachitchyan November 9, 2023 19:54
@tardis-bot
Copy link
Contributor

*beep* *bop*

Hi, human.

The docs workflow has succeeded ✔️

Click here to see your results.

@AlexHls AlexHls marked this pull request as ready for review November 27, 2023 22:08
@AlexHls
Copy link
Member Author

AlexHls commented Nov 29, 2023

This should now be ready for final review @chvogl @sonachitchyan

Copy link
Contributor

@andrewfullard andrewfullard left a comment

Choose a reason for hiding this comment

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

Frankly, I don't understand exactly how the solver works. However, the code is well-written so I will approve it on the basis with the caveat that I don't see any examples showing the functionality of the solver working as expected (e.g. compared to some reference).

My other major comment is that given the computational intensity of this solver, the code would really benefit from being made Numba-compatible in the future.

@wkerzendorf
Copy link
Member

we merged it in a new PR

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.

9 participants