-
Notifications
You must be signed in to change notification settings - Fork 19
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
Improve Fz2df conversion #202
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @mondracek. Please add simple tets to the methods from common.py
you are updating.
ppafm/common.py
Outdated
w[:-1] -= t[1:]*df + df2 #coefficients to multiply F[i+1] | ||
return w/(np.pi*dz) | ||
|
||
def Fz2df( F, dz=0.1, k0 = params['kCantilever'], f0=params['f0Cantilever'], A = 0.1, units=16.0217656 ): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest changing A to the amplitude
to be more explicit.
Also, please mention in the comments that the amplitude
is peak-to-peak (not the usual definition). Is that correct, @ProkopHapala?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is peak-to-peak. I would actually change it if were just up to me, but that would be a too hard blow to backward compatibility for others I guess.
ppafm/common.py
Outdated
@@ -230,40 +230,72 @@ def add_arguments(self, arg_names): | |||
# ============================== Pure python functions | |||
# ============================== | |||
|
|||
def getDfWeight( n, dz=0.1 ): | |||
def getDfWeight( A, dz = 0.1): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rename it to get_df_weights
…_weight. - renamed getDfWeight in common.py to get_df_weight - renamed argument A to Amp (in get_df_weight) or to amplitude (in Fz2df) - corrected substantial mistake in get_df_weight(Amp, dz) - introduced get_simple_df_weight(n, dz) as an alternative to get_df_weight - corrected and improved some comments in common.py - updated tests/test_common.py - modified scripts in ppafm/cli/fitting to pass test, probably still broken
- ppafm/cli/plot_results.py - ppafm/common.py
ppafm/common.py
Outdated
if(n>1): | ||
f[1:-1] = np.sqrt(1 - t[1:-1]*t[1:-1]) | ||
f2[1:-1] = (np.arccos(-t[1:-1]) - t[1:-1]*f[1:-1]) / 2.0 | ||
f2[0] = np.pi #end point for t>=1 must be as if t=1 even if t>1 | ||
f2[0] = np.pi/2 #end point for t>=1 must be as if t=1 even if t>1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pi/2
belongs here, not just pi
.
I have renamed the function |
To sum up what should be still considered for review in this PR:
I'm pro keeping both. You have nice doc-string explaining the use case
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. |
@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 |
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"). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I approve, @mondracek, thanks a lot for your work!
Do not forget to use "Squash and merge" when merging this PR.
I have defined
lvec_df
inppafm/cli/plot_results.py
so that it contains a modified copy oflvec
. 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). Thislvec_df
is then used when savingdf.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
andFz2df_tilt
inppafm/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 functiongetDfWeight
. 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 theGraphene/
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 thegetDfWeight
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 fordf.xsf
in this branch.