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

NEW feature: intermediate scattering function #1042

Open
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

Roy-Kid
Copy link

@Roy-Kid Roy-Kid commented Nov 30, 2022

Description

Please assist me in improving the code @tommy-waltmann
To calculate time-dependent intermediate scattering function. Here are the refs:

  1. https://en.wikipedia.org/wiki/Dynamic_structure_factor
  2. https://www.lehigh.edu/imi/teched/AtModel/Lecture_11_Micoulaut_Atomistics_Glass_Course.pdf

Motivation and Context

Here we add a class derived from StaticStructureFactorDirect to calculate the time-dependent intermediate scattering function.
Resolves: #1040

How Has This Been Tested?

This code has not been tested before complete

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • [ x] New feature (non-breaking change which adds or improves functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation improvement (updates to user guides, docstrings, or developer docs)

Checklist:

  • [ x] I have read the CONTRIBUTING document.
  • [x ] My code follows the code style of this project.
  • I have updated the documentation (if relevant).
  • I have added tests that cover my changes (if relevant).
  • All new and existing tests passed.
  • I have updated the credits.
  • I have updated the Changelog.

@Roy-Kid Roy-Kid requested a review from a team as a code owner November 30, 2022 04:18
@Roy-Kid Roy-Kid requested review from Charlottez112 and removed request for a team November 30, 2022 04:18
pre-commit-ci bot and others added 8 commits November 30, 2022 04:18
… store the results of each accumulate at the python-end; so we need to create a data structure to save multiple getBinCounts of self and distinct function and convert it to (n_frames, nbins) array
@tommy-waltmann
Copy link
Collaborator

Hi @Roy-Kid, thanks for getting such a quick start! This is great and I can tell this took significant effort on your part to write. Before I get into more detailed comments, I want to talk about some conceptual things related to the intermediate scattering function.

The first thing is that the intermediate scattering function is not a static structure factor, it is dynamic in that it needs data from multiple different times in order to compute. At the same time, it shares the same method of sampling vectors in k-space with the StaticStructureFactorDirect class. The intermediate scattering function can also be thought of as a structure factor because it is the autocorrelation of the direct structure factor. With those ideas in mind, I want to alter the inheritance hierarchy among structure factor classes so the intermediate scattering function can fit in nicely. I will implement those changes since they alter the structure of previously developed code somewhat.

The second thing is about whether or not we should be able to accumulate the intermediate scattering function. In the MSD code, the calculation can be accumulated over more points because each point's MSD can be computed independent of the other points. In the code for the other structure factors, the calculation can be accumulated over many frames because each frame can be computed independent of the other frames, and the histogram freud computes internally can have more counts added to it. The intermediate scattering function, by contrast to the previous two calculations, needs all positions at all times to compute, which means accumulation conceptually does not make sense.

Going forward, I will jump in and alter the inheritance hierarchy of the structure factor classes, remove the ability to accumulate from the class, and make some other small changes to the API. Then, if you wish, you can jump back in and make some of the more detailed changes like updating the documentation, adding more tests, validating the calculation on your use case, etc.

Once again, thanks for taking the initiative with this feature, I'm sure other people will find use for it as well!

@Charlottez112
Copy link
Member

Once again, thanks for taking the initiative with this feature, I'm sure other people will find use for it as well!

Yes I will. Thanks guys!

@tommy-waltmann
Copy link
Collaborator

One more conceptual thing that I forgot to mention before: There should be no need for use of a neighbor list in this calculation. Since the sums are over all particles, we already know that all particles are neighbors of each other. NeighborQuery/NeighborList objects are for defining/finding neighbors when it isn't already obvious which particle pairs the calculations need to consider.

It will likely, in fact, slow down the IntermediateScattering implementation to use a NeighborQuery because the NeighborQuery will build a complex data structure and find neighbors according to search parameters, but it will just end in all particle pairs being neighbors, and it would be much faster to do a nested loop over all particle indices.

@Roy-Kid
Copy link
Author

Roy-Kid commented Dec 7, 2022

Going forward, I will jump in and alter the inheritance hierarchy of the structure factor classes, remove the ability to accumulate from the class, and make some other small changes to the API.

Thanks for all your correction! I have some misunderstandings about the intermediate scattering function. Now it seems that it is not a simple combination of MSD and static scattering function (even if some methods can be reused). I'll leave the code to you next. Thank you for your adjustment in the code architecture.

@tommy-waltmann
Copy link
Collaborator

Hi, @Roy-Kid at this point I have finished restructuring the code so that the calculation will fit correctly into freud's structure.. Everything will compile, but the implementation is wrong. Are you willing to give the implementation another try? It should only involve changing the compute function on the C++ side of the code.

@Roy-Kid
Copy link
Author

Roy-Kid commented Dec 18, 2022

Hi, @Roy-Kid at this point I have finished restructuring the code so that the calculation will fit correctly into freud's structure.. Everything will compile, but the implementation is wrong. Are you willing to give the implementation another try? It should only involve changing the compute function on the C++ side of the code.

I'm so sorry for replying late. I'm busy with my mid-term assessment and PhD application, so I don't have time to develop the code these days. If you have time, could you please continue to finish the work, or wait for me until the next year February?

Sorry again for the delay...

Roy-Kid and others added 2 commits December 25, 2022 20:58
@Roy-Kid
Copy link
Author

Roy-Kid commented Dec 25, 2022

Hi, @tommy-waltmann. I try to implement the code of compute, and it looks like done. But I get a fatal error at getSelfFunction of IntermediateScattering.cc line 121. I am confused with those reduce functions, so I don't figure out how to fix this problem. Could you please help me with this?

@tommy-waltmann
Copy link
Collaborator

@Roy-Kid the issue was in access out-of-bounds memory in two spots, I've made some changes and there are no more crashes. The code should be in a place where you can continue with the implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

A Request for Intermediate Scattering function
3 participants