Probe-Particle / ppafm

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

Improve Fz2df conversion #202

Closed mondracek closed 11 months ago

mondracek commented 12 months ago

I have defined lvec_df in ppafm/cli/plot_results.py so that it contains a modified copy of lvec. The modification consists basically in subtracting the amplitude from the height (defined by the third vector) and adding half of the amplitude to the origin (the "zeroth" vector). This lvec_df is then used when saving df.xsf (requested by the --save_df option). This will close #199. Besides that, I have improved the numerical conversion from force to frequency shift itself, as implemented by the functions Fz2df and Fz2df_tilt in ppafm/common.py. Well, changed at least, I hope that it is an improvement, although I made the numerical procedure more complicated. In particular, I have totally rewritten the function getDfWeight. My intention was to make it work for very small amplitudes. The former solution could become imprecise if the amplitude was comparable to the grid step, thus extending over only very few grid points. If the amplitude equaled the grid step or if it was even smaller, the algorithm failed completely. My proposed solution should return a frequency shift proportional to the gradient of the force (estimated from forces on two consecutive grid points) for amplitudes smaller than or equal to the grid step. Moreover, while the former solution always rounded the amplitude to some integer multiple of grid step, now the conversion result is a continuous function of the (real-typed) amplitude, even though, of course, a discrete convolution will still be done. Not sure I would have implemented the new way of Fz2df if I had realized from the beginning how complicated the formulae were going to be. Nevertheless, here it is. Please decide if you like the change. It is going to impact all AFM images, though I have checked using the Graphene/ example that the changes are very small, usually not visible by eye at all except for extremely small amplitudes. I should probably write up a description of the algorithm in LaTEX and put it up somewhere, or perhaps put it to the Wiki. So far, however, I just described it by comments in the code of the getDfWeight function. Unfortunately, the math rendered in the plain text format is very hard to read, I am afraid. If you do not like the new force to df conversion, I will put back the old solution or push some alternative less radically different from the old one, keeping just the height correction for df.xsf in this branch.

mondracek commented 11 months ago

I have renamed the function getDfWeight to get_df_weight, as requested by @yakutovicha . I have also updated the tests in tests/test_common.py so that the function is tested and passes the test. A test for an alternative function get_simple_df_weight was added too; more about this function below. Good that I did it, I have discovered a mistake in get_df_weight during the process. This mistake is now corrected, it was in this line: f2[0] = np.pi/2 #end point for t>=1 must be as if t=1 even if t>1 I forgot the division of pi by 2, which resulted in a completely nonsensical weight function, although, surprisingly, the resulting images looked correct even with the mistake. I have also renamed the functional argument A to amplitude in the argument lists of functions Fz2df and Fz2df_tilt, as suggested by @yakutovicha. I did not use the name amplitude in functions get_df_weight and get_simple_df_weight, as it would look too verbose in the formulas, but I used Amp there as a compromise naming. I have noticed that the function getDfWeight was called in ppafm/ocl/AFMulator.py. This call needed to be updated too, which escaped my attention in the first commit. Seeing that the function (now get_df_weight) was also needed for the ocl version, I was worried that my new solution might not suitable for that. I gather that the ocl version is expected to be particularly fast and efficient and such abominations of numerical mathematics like the arccos function I have used in my new get_df_weight could be a problem. Perhaps the worry was not really substantiated, the difficult formulas are evaluated on just a handful grid points in get_df_weight. Anyway, just to be safe in not spoiling anything, I have implemented yet another version of the weight function, which I call get_simple_df_weight. The implementation is more similar to the original getDfWeight and, in particlular, avoids the arccos function. This get_simple_df_weight is now called in the ocl version. In case you want the cli and ocl versions to give numerically identical results, we can change this and pick one of the function get_df_weight or get_simple_df_weight to be used everywhere. Just careful, there are different arguments to the two functions. While get_df_weight takes the amplitude (Amp) in angstroms, get_simple_df_weight accepts number of grid points n corresponding to the amplitude (as the old getDfWeight did). Finally, I was forced to modify too scripts in the ppafm/cli/fitting/ directory, namely ppafm/cli/fitting/plotZ.py and ppafm/cli/fitting/plotLine.py, because the both call the Fz2df function. Once I modified them, however, they became subject to the automatic tests every time I tried to commit this branch. The tests revealed that there are more problems with this pieces of code than just those related to Fz2df. It seems they have been neither used nor tested for a long time and relied on now abandoned features of older versions of the code. Namely, the new way of parameter loading was never implemented there. I have made minimal fixes to push these scripts through the tests but they are probably still wrong. Does anyone know what exactly they are supposed to do and how to test them? @ProkopHapala perhaps? If yes, please help me correct these scripts. Otherwise, if nobody remembers what the things in the ppafm/cli/fitting/ directiory are supposed to do, I suggest we remove the whole directory and the *.py scripts it contains from the main branch.

mondracek commented 11 months ago

To sum up what should be still considered for review in this PR:

  1. Should we keep both get_df_weight and get_simple_df_weight? I am for keeping them but maybe I am too attached to the work I have done. I am open to the argument that one of them be eliminated for the sake of simplicity, if you feel so.

I'm pro keeping both. You have nice doc-string explaining the use case

  1. The Amp vs amplitude naming controversy. I prefer to keep the names as they are now. If anyone prefers a change, please pull this branch, make the change, look at the result, decide whether you think it makes the code more legible, and if you are happy with it, push it back here. As long as it is just renaming and does not break any functionality, I will approve it.

Keep it as you have it. I relay on your judgement.

In general I think it is not worth it to spend time discussing such minor things as name of some variable. Although I think some naming convetions are nice in general, I don't consider it crucial. Our time is limited. If somebody (you, me, anybody) does some cosmetic changes, which are not super-controversial, I think it is better just do it, and thrust the person he did it according to his best judgement, rather than discuss it in detail. because to me it seems that the time I woould need to read the code and understand all your arguments would be longer than actually doing it myself. And therefore if everybody discuss it, it is like if everybody would duplicate the work. Which may be worth it for some critical and controversial part of core functionality. But for somthing as cosmetic as naming functions and variables I think it it not worth it.

mondracek commented 11 months ago

@yakutovicha or anyone else, is there anything that needs to be done with this PR yet? If not, I would like to merge it.

ProkopHapala commented 11 months ago

@yakutovicha or anyone else, is there anything that needs to be done with this PR yet? If not, I would like to merge it.

please go ahead Martin

mondracek commented 11 months ago

Okay then, @ProkopHapala and @yakutovicha , please give me a formal approval (click on "Add your review", go to "Review changes", select "Approve" and confirm by clicking on "Submit review").