Initial SPA1 Implementation for 1D Model#7
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7 +/- ##
==========================================
+ Coverage 54.26% 54.54% +0.27%
==========================================
Files 42 42
Lines 5998 6041 +43
Branches 800 808 +8
==========================================
+ Hits 3255 3295 +40
- Misses 2395 2398 +3
Partials 348 348 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Huh, interesting, the AIMSWISS test is failing on the debug build with the following stack trace. At line 181 of file SelectionModule.f90
Fortran runtime error: Index '6' of dimension 1 of array 'blockpop' above upper bound of 3
Error termination. Backtrace:
#0 0x7fea74823e59 in ???
#1 0x7fea74824a71 in ???
#2 0x7fea74825082 in ???
#3 0x55fcb49103a5 in perform_stochastic_selection
at /home/runner/work/openfms/openfms/src/modules/SelectionModule.f90:181
#4 0x55fcb4945aa9 in __selectionmodule_MOD_fms_stochasticcollapse
at /home/runner/work/openfms/openfms/src/modules/SelectionModule.f90:111
#5 0x55fcb4a38a73 in fms_dynamics_
at /home/runner/work/openfms/openfms/src/dynamics.f90:81
#6 0x55fcb4766c63 in openfms
at /home/runner/work/openfms/openfms/src/openfms.F90:137
#7 0x55fcb4766d07 in main
at /home/runner/work/openfms/openfms/src/openfms.F90:12@adamjharrison when you get a chance, can you look at the code and understand how it could be influenced by your changes? EDIT: Don't sweat about it too much, it's likely it's an pre-existing bug that's somehow exposed here? |
danielhollas
left a comment
danielhollas
left a comment
There was a problem hiding this comment.
@adamjharrison thanks, this looks great!
I mostly have questions about naming, maybe that's something we can discuss in person next week.
I haven't verified all the various formulas, as I trust you did a good job there. But it would be great if you could update the PR description with a detailed explanation of how you tested everything.
It might be great actually to write some unit tests for this, we can discuss how to do it next week.
|
Implementation verified by comparing the first order SPA1 term calculated by the code to wolfram alpha values for scenarios where gaussians are overlapping in both regions of changing and constant SOC. |
danielhollas
left a comment
danielhollas
left a comment
There was a problem hiding this comment.
Amazing, thanks! Just a couple of small comments regarding the test.
| @@ -0,0 +1,49 @@ | |||
| &control | |||
| ! Input for some model | |||
There was a problem hiding this comment.
Can you please expand this comment? Just explain what this test is testing. :-)
| NumTriplets=1 | ||
| zAllText=.true. ! Output all text-based files | ||
| NumInitBasis=1 | ||
| SPA1_SOC_model=.true. !First order saddle point approximation for 1D model System |
There was a problem hiding this comment.
This is a crucial config, can you move it to the top of the file?
There was a problem hiding this comment.
Can you please remove this file. Also let's add .DS_Store in .gitignore so this issue doesn't happen again.
(in any case, great to see that local development works on MacOS :-) )
There was a problem hiding this comment.
Perfect I've removed it and added .DS_Store to the .gitignore file
3 participants
Initial implementation of SPA1 interstate and intrastate terms for 1D Model (G. Granucci, M. Persico, G. Spighi, J. Chem. Phys., 2012, 137, 22A501)
Implementation verified by comparing the first order SPA1 term calculated by the code to wolfram alpha values for scenarios where gaussians are overlapping in both regions of changing and constant SOC.