search

mattwthompson / scattering

Functions for analyzing molecular simulations according to scattering experiments
MIT License
9 stars 9 forks source link

Update static structure function #23

Closed rmatsum836 closed 3 years ago

rmatsum836 commented 4 years ago

I'm currently trying to calculate S(Q) for acetonitrile, and I'm not reproducing the curves from our collaborators. This could be due to a number of factors but I figured I'd take a look at the function.

It seems like the function is using the Ashcroft-langreth (though correct me if I'm wrong) formalism for calculating partial structure factors: http://isaacs.sourceforge.net/phys/scatt.html. Based on the equations in the link, I think I found a couple errors.

First, both terms in the denominator were being squared when I think only the form factor should be squared. Next, it seems that the square root should be taken of the concentration terms. Lastly, the numerator of the summation should have (S(Q)_partial + 1) instead of just S(Q).

Upon testing however, I'm still not reproducing the scattering results so this is a WIP. May ask others for a second pair of eyes.

rmatsum836 commented 4 years ago

The calculation of the total RDF from the partials involves a double summation . The correct function to use in this case is itertools.product instead of itertools.combinations_with_replacement. After making this change, I'm now getting the correct g(r).

Now I need to see if this change fixes the S(Q) as well.

rmatsum836 commented 4 years ago

Think I have the Faber-Ziman method for partial structure factor figured out, which I got from this dissertation: https://openscholarship.wustl.edu/etd/1358/. For now Faber-Ziman is the default method.

mattwthompson commented 4 years ago

You've put more thought into this than I did. The formula I used originally was just copied from some paper without doing the necessary verification.

Can you add some unit tests, and also a plot showing the difference in the methods (doesn't need to be in the code)?

rmatsum836 commented 4 years ago

I will add unit tests and documentation today. Right now the faber-ziman method is the only one I've truly validated (I've compared it to scattering results from Yuya and Chaewoo).

rmatsum836 commented 4 years ago

I removed the Ashcroft-Langreth method for now as I wasn't able to get it working and I'd like to get this merged.
Attached is a structure factor for SPC/E water using the faber-ziman method.

sq_comparison

rmatsum836 commented 4 years ago

For validation, here is S(Q) for acetonitrile using the Faber-Ziman Method which can be compared with this paper: https://www-sciencedirect-com.proxy.library.vanderbilt.edu/science/article/pii/S0167732216331701?via%3Dihub

Note that the S(Q) in the paper is S(Q) - 1, whereas I'm plotting S(Q). The paper also uses units of angstroms, and I plotted in nm.

image

mattwthompson commented 4 years ago

Certainly passes the eye test!

rmatsum836 commented 3 years ago

After discussions with @ramanishsingh I think the docstring should be updated to make explicitly clear that the structure factor is being computed through the Fourier transform of the RDF. Also the method argument should be renamed to something like weighting_factor.

rmatsum836 commented 3 years ago

Recent commit clarifies that the structure factors is computed from the Fourier transform of the RDF. Additionally, the method argument has been renamed to weighting_factor to be more explicit. @ramanishsingh could you review this PR once more before we merge?

ramanishsingh commented 3 years ago

Sure, I will review it.

ramanishsingh commented 3 years ago

The results with the updated function (weighting_factor) match my old results. However, I faced some issues with the dependencies. When I executed: pip3 install -r requirements.txt it gave me the following error:

ERROR: Could not find a version that satisfies the requirement itertools (from -r requirements.txt (line 4)) (from versions: none)
ERROR: No matching distribution found for itertools (from -r requirements.txt (line 4))

So I just removed it from requirements.txt.

Also, progressbar (package) should be included in the requirements, however I think it's not being used in scattering.py, right?

rmatsum836 commented 3 years ago

The issue with requirements.txt is most likely due to MDTraj, are you able pip install that package alone?. And thanks for pointing out that progress bar should be added into the requirements.

rmatsum836 commented 3 years ago

Progressbar issue documented in #17

ramanishsingh commented 3 years ago

The issue with requirements.txt is most likely due to MDTraj, are you able pip install that package alone?. And thanks for pointing out that progress bar should be added into the requirements.

pip3 install -r requirements.txt is working now!

rmatsum836 commented 3 years ago

@ramanishsingh I just added information regarding the form factors into the doc string. If everything looks okay, I'll go ahead and merge.

ramanishsingh commented 3 years ago

Thanks! Looks good. Please merge.

rmatsum836 commented 3 years ago

Actually, @mattwthompson will have to merge or give me write access if he wishes.

Here is a summary of this PR:

mattwthompson commented 3 years ago

I'll let you merge given that you and @ramanishsingh seem to be happy with it.

I've added you as a collaborator, and I'm open to moving this repo to another owner if needing me to respond to notifications is a blocker for your science

rmatsum836 commented 3 years ago

I'll let you merge given that you and @ramanishsingh seem to be happy with it.

I've added you as a collaborator, and I'm open to moving this repo to another owner if needing me to respond to notifications is a blocker for your science

Thanks! And no worries, we should be okay now that I'm a collaborator.