diffpy / diffpy.srreal

Python library for PDF calculation and assortment of real-space utilities.
Other
8 stars 21 forks source link

Identical PDF outputs with large Qmax #24

Open lanikaling opened 3 years ago

lanikaling commented 3 years ago

Hi @pavoljuhas @sbillinge @st3107, When I generate PDF using diffpy.srreal.pdfcalculator.PDFCalculator with parameters as following: rmin = 1.5 rmax = 30 rstep = np.pi / 23.0 qmin = 0.5 qmax = 23 qdamp = 0.04 qbroad = 0.01 adp = 0.008 Then for any structure loaded from CIF, the PDFs generated for this structure are identical for any qmax>23. I have included an example code for your convenience. If you change qmax for some number larger than 23 and use the same cif file, then you will find the output g's are the same.

from diffpy.structure import Structure, loadStructure
from diffpy.srreal.pdfcalculator import PDFCalculator
s = loadStructure(cif_dir)
s.Uisoequiv = 0.008
cfg = {'qmax': 23, 'qmin': 0.5, 'rmin': 1.5, 'rmax': 30, 'rstep': np.pi / 23.0, 'qdamp': 0.04, 'qbroad': 0.01}
pc = PDFCalculator(**cfg)
r, g = pc(s)

Thank you, Ling

lanikaling commented 3 years ago

Hi,

We found out that the condition that leads to identical generated PDF is actually rstep*qmax>pi. We think it might come from the Fourier Transform, and we wonder if there is any if/else statement in the code. Thanks.

Best, Ling

pavoljuhas commented 3 years ago

Hi Ling, yes, this is an expected behavior. A discrete r-sampling can only capture Q-frequencies up to the pi/rstep limit. The Qmax correction to PDF is done by cropping away frequencies above the Qmax choice. This however makes no difference when Qmax is larger than the Q limit available from r-sampling.

Here is the check of these limits in the code - https://github.com/diffpy/libdiffpy/blob/906cda4a5a663a464d96100c5094927c834f3bd2/src/diffpy/srreal/PDFCalculator.cpp#L154-L158

lanikaling commented 3 years ago

Hi Pavol, Thanks for the explanation. It makes sense. @st3107 and I suggest adding a warning message here, which tells the users that the output PDF is the same as the one with Qmax=pi/rstep and we suggest they choose a smaller rstep.

@st3107 Do we need to get into the C++ code? As Simon says, we could handle this issue by checking their input values. Shall we try to fix it first, and ask Pavol if we are not able to?

sbillinge commented 3 years ago

@lanikaling let's try and make a PR ourselves on this, we shouldn't need Pavol's help. If we need him we can reach out later.

st3107 commented 2 years ago

@sbillinge @lanikaling I think that we can add this in the documnet instated of the code. The reason is that if we add it in the code, every time the PDF is calculated, there will be a warning message printed out and user will see tons of same warning when doing the refinemnt.

sbillinge commented 2 years ago

I think the warning is only printed if the condition is violated, right? Which will normally be never....it is just we ran into it in this context because we had a very short range we were computing over. Having it in the documentation is good too.

On Tue, Oct 26, 2021 at 12:06 PM Songsheng Tao @.***> wrote:

@sbillinge https://github.com/sbillinge @lanikaling https://github.com/lanikaling I think that we can add this in the documnet instated of the code. The reason is that if we add it in the code, every time the PDF is calculated, there will be a warning message printed out and user will see tons of same warning when doing the refinemnt.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/diffpy/diffpy.srreal/issues/24#issuecomment-952089575, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABAOWUIK57W2O6FVKAOPBF3UI3GZBANCNFSM5B26SDFA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

-- Simon Billinge Professor, Columbia University Physicist, Brookhaven National Laboratory