-
Notifications
You must be signed in to change notification settings - Fork 44
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
Add Analytic Electron-Ion Coupling to CDI #586
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #586 +/- ##
=========================================
+ Coverage 93.7% 93.7% +<.1%
=========================================
Files 377 380 +3
Lines 17665 17779 +114
=========================================
+ Hits 16554 16667 +113
- Misses 1111 1112 +1 |
@kgbudge Will you please look over this PR? |
@clevelam Rerunning the the checks on trinitite didn't fix those issues. I'll checkout your PR and attempt to run it manually. Something may have changed on trinitite during this week's DST. |
@clevelam As I expected, something has changed on trinitite. I'll consider that a separate issue and won't count the warnings from that machine against this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@clevelam This looks pretty good to me. I've requested a few minor changes, but I don't consider them to be blockers. I would like to get some feedback from @kgbudge, @jhchang-lanl and @warsa. It seems they should have some say in this design as they may need to adopt it.
As you mention this can assume a mixture of ion species underneath - it would be good to have a test case that accepts such a mixture. |
@clevelam This looks to be very close to being done. Ping me when you think it is ready. |
+ Fix cmake logic in find OpenBLAS refs #1420
Last set of changes looks good. All tests pass. |
Background
Purpose of Pull Request
Description of changes
Status