Probe-Particle / ppafm

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

Correct sign of densities and potentials in KPFM #251

Closed mondracek closed 8 months ago

mondracek commented 8 months ago

Fixes #250. What I have done to address the sign issue itself can be best understood from the code changes I think. Besides that

  1. The main of ppafm/cli/generateElFF.py was creating two copies of the electrostatic potential before calculating LCPD: v_v0_aux = electrostatic_potential.copy() and v_v0_aux2 = electrostatic_potential.copy(). Having the same field written in three places made the code look rather messy to me and I found it difficult to keep track of the required sign changes in such a mess so I did away with those copies. As I understood it, they were needed because the potential2forces_mem function in ppafm/fieldFFT.py would remove the array with the potential after the potential had been used, unless the function had been called with the deleteV=False argument. In order to address this, I have therefore introduced the deleteV argument to the function computeElFF from ppafm/HighLevel.py so that the deleteV=False option could be passed from generateElFF.py to potential2forces_mem through computeElFF.
  2. The FFkpfm_tVs0 term was scaled by the tip charge abs(PPU.params.["charge"]) here https://github.com/Probe-Particle/ppafm/blob/18a5d8a68158e5ffc76d7988bb3e9a6a3b86ff2d/ppafm/HighLevel.py#L160 I did not think this made any sense so I removed this scaling: https://github.com/Probe-Particle/ppafm/blob/aaeedbcbabab2c82453ff283379b4a4e21720071/ppafm/HighLevel.py#L160 @aureliojgc, can you comment on what idea was behind it? Anyway, as I removed it, I changed the meaning of the numerical tip polarizabilities defined here: https://github.com/Probe-Particle/ppafm/blob/aaeedbcbabab2c82453ff283379b4a4e21720071/ppafm/cli/generateElFF.py#L126-L137 I did not change the numbers (only the signs) as it seemed to me that before I removed the scaling, the FFkpfm_tVs0 contribution was always negligible with respect to the FFkpfm_t0sV contribution for default tip polarizability and typical tip charge, which did not seem right. Now this relative contributions of FFkpfm_tVs0 and FFkpfm_tVs0 somewhat improved as to become more equalized.
codecov[bot] commented 8 months ago

Codecov Report

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

Comparison is base (18a5d8a) 46.40% compared to head (00f61c3) 46.34%.

Files Patch % Lines
ppafm/cli/generateElFF.py 0.00% 9 Missing :warning:
ppafm/HighLevel.py 50.00% 2 Missing :warning:
ppafm/PPPlot.py 0.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #251 +/- ## ========================================== - Coverage 46.40% 46.34% -0.06% ========================================== Files 35 35 Lines 5185 5170 -15 ========================================== - Hits 2406 2396 -10 + Misses 2779 2774 -5 ``` | [Flag](https://app.codecov.io/gh/Probe-Particle/ppafm/pull/251/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Probe-Particle) | Coverage Δ | | |---|---|---| | [python-3.10](https://app.codecov.io/gh/Probe-Particle/ppafm/pull/251/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Probe-Particle) | `?` | | | [python-3.11](https://app.codecov.io/gh/Probe-Particle/ppafm/pull/251/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Probe-Particle) | `?` | | | [python-3.12](https://app.codecov.io/gh/Probe-Particle/ppafm/pull/251/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Probe-Particle) | `?` | | | [python-3.7](https://app.codecov.io/gh/Probe-Particle/ppafm/pull/251/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Probe-Particle) | `?` | | | [python-3.9](https://app.codecov.io/gh/Probe-Particle/ppafm/pull/251/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Probe-Particle) | `46.34% <14.28%> (+0.04%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Probe-Particle#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

mondracek commented 8 months ago

This PR now also closes #252. On the one hand, I understand this is a somewhat non-standard work flow as the two issues, #250 and #252, have nothing in common except they both need to be solved in order to generate the LCPD image for the paper. On the other hand, we need the figure for the paper urgently and the commit that solves #252 changes literary just one line of code. As long as the change is uncontroversial, doing all the motions with a separate pull request would be a waste of time and effort.

Tl;dr: Please also check the second commit in this PR.