NaI analytic potential for FSSH#178
Conversation
Current implementation works for 1 particle in x coordinate which is considered to be the r=r1-r2 coordinate.
The implementation was briefly tested. More thorough testing is necessary.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #178 +/- ##
==========================================
- Coverage 92.84% 92.83% -0.01%
==========================================
Files 46 47 +1
Lines 6720 6787 +67
Branches 754 762 +8
==========================================
+ Hits 6239 6301 +62
- Misses 469 474 +5
Partials 12 12
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
This looks great, amazing work @JanosJiri! 👏 Really excited for this.
I'll have a closer look at the implementation later, but it looks good overall from high level.
Thank you for a very clean code and comments.
Besides some minor stuff I noticed, it would be great if you could add at least one basic test.
Also, please run ./autoformat.py which should autoformat the code. You might need to split some of the long lines manually, since the formater isn't clever enough for that.
One testing trajectory was added. The test is performed on dotprod, prob, pop, distances and PES.
|
Fprettify applied and one test added. I also processed all the changes requested except for one where I have a question for you. Once we resolve this, the code will be ready for merging. |
|
Excellent, thanks!
Hmm, I don't see the question anywhere. Did you post it on Github? (Github review UI is sometimes super confusing so perhaps I just missed it) |
Yes, it's on Github but it probably got lost in the conversation. I was asking if you really want to name the file |
|
Thanks, the comment is marked as "pending", you'll need to "Finish the review" to publish it. |
|
@JanosJiri apologies for the delay while I was away. This looks really great, thanks for adding a test and extensive comments. Just one question, could you reduce the number of steps in the test? We generally try to keep the number of steps to the absolute minimum for numerical stability, to reduce the chance that the test will break with different compiler versions for example. Also, the test now takes more than 5s, which might not seem like much, but since we have a lot of them it adds up. When you modify the make makeref TEST=SH_NaIIn most tests with only do one step. Maybe here we want a bit more so we can test the value of NACM? But I think you anyway see it in |
2 participants
Analytic potential for NaI implemented for surface hopping dynamics. The original potential is adiabatic but was adiabatized in the code. Everything even with the source articles is described in the code. 3ps of dynamics can be done in 8 seconds when one is not writing too much on the disk. Accessing the disk is the most demanding part of the simulation. The implementation was compared to the exact QD.
