Skip to content
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

Correct sign of densities and potentials in KPFM #251

Merged
merged 5 commits into from
Jan 15, 2024
Merged

Correct sign of densities and potentials in KPFM #251

merged 5 commits into from
Jan 15, 2024

Conversation

mondracek
Copy link
Collaborator

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
    FF += (np.sign(PPU.params["charge"]) * FFkpfm_t0sV - FFkpfm_tVs0) * abs(PPU.params["charge"]) * PPU.params["Vbias"]
    I did not think this made any sense so I removed this scaling:
    FF += (PPU.params["charge"] * FFkpfm_t0sV - FFkpfm_tVs0) * PPU.params["Vbias"]
    @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:
    if common.params["probeType"] == "8":
    drho_kpfm = {"pz": 0.045}
    sigma = 0.48
    print(" Select CO-tip polarization ")
    if common.params["probeType"] == "47":
    drho_kpfm = {"pz": 0.21875}
    sigma = 0.7
    print(" Select Ag polarization with decay sigma", sigma)
    if common.params["probeType"] == "54":
    drho_kpfm = {"pz": 0.250}
    sigma = 0.67
    print(" Select Xe-tip polarization")
    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.

@mondracek mondracek added the bug label Jan 9, 2024
@mondracek mondracek self-assigned this Jan 9, 2024
Copy link

codecov bot commented Jan 9, 2024

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 ⚠️
ppafm/HighLevel.py 50.00% 2 Missing ⚠️
ppafm/PPPlot.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            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 Coverage Δ
python-3.10 ?
python-3.11 ?
python-3.12 ?
python-3.7 ?
python-3.9 46.34% <14.28%> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mondracek
Copy link
Collaborator Author

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.

@mondracek mondracek merged commit cae4e39 into main Jan 15, 2024
13 checks passed
@mondracek mondracek deleted the kpfm branch January 15, 2024 17:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

KPFM is wrong because of sign issues
3 participants