Probe-Particle / ppafm

Classical force field model for simulating atomic force microscopy images.
MIT License
49 stars 18 forks source link

LCPD for general ranges of bias, amplitudes, tip charge and cantilever stiffness #278

Closed mondracek closed 2 months ago

mondracek commented 4 months ago

Resolves #272 and resolves #245 too.

Note that from now on, the files LCPD.xsf and *LCPD*.png will be placed in a Q?.??K?.??/Amp?.??/ rather than directly in the working directory, because, in general, more LCPD results for different Q, k, A can be calculated in one run (consequence of implementing issue #272). No restriction on --Vrange now (implementing issue #245), except that of course at least 3 different bias values are needed to evaluate LCPD . Regression of quadratic polynomial (for the Kelvin parabola) with the least-square-of-error criterion will be used when more than three values of bias are available. The bias range in ppafm-plot-results --Vrange still needs to be compatible with the range in ppafm-relaxed-scan --Vrange, of course.

mondracek commented 4 months ago

Tested on CH3Br, and FFPB. The resulting *LCPD*.png files are virtually the same as with the previous implementation. They are not exactly bit-wise identical if different --Vrange are used, but that is expected.

mondracek commented 4 months ago

Continuous integration fails with test-package v 3.9 and higher. Probably nothing to do with the part of the code I have modified. @yakutovicha, you probably understand the best how this continuous integration is supposed to work and how to manage it. Can you give me a hint what may be going on?

Edit: Sorry, the problem was likely just that the main branch on which I have had based this was not up to date. I have force-pushed an amendment. Another edit: Tests still failing, even after re-basing on an up-to-date main branch.

mondracek commented 4 months ago

What I have found about the continuous-integration tests:

What happened as a side effect of me finding out and what I am going to do now:

NikoOinonen commented 3 months ago

@mondracek I merged the PR for fixing the CI tests. If you merge main into this branch, the tests should pass.

mondracek commented 3 months ago

Sorry, I rebased and force-pushed instead of merging with main, which could be undesired in some cases and I am going to avoid it next time, but that's a side point here ... The final result should be the same. What is unfortunately the important point is that _The tests are still failing, even with only examples/PTCDA_Hartreedz2/ being tested.

NikoOinonen commented 3 months ago

@mondracek There is something odd going on with the tests. I simply reran them and they passed, so it seems not to be deterministic. If you are confident that this PR cannot be influencing this, then I would just go ahead with it now.

mondracek commented 3 months ago

Okay. I will wait for one approving review before merging. Preferably from @aureliojgc , as he knows the details of the LCPD calculations, but if Aurelio does not have time for it, anyone who feels what I did makes sense can approve.

NikoOinonen commented 2 months ago

@mondracek Shouldn't this be merged or is there some problem?

mondracek commented 2 months ago

I thought I should wait for @aureliojgc to finish the tests he mentions above but then I decided to just go ahead and merge it.