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

Adding PDF calculation function #500

Merged
merged 36 commits into from
Oct 23, 2023
Merged

Adding PDF calculation function #500

merged 36 commits into from
Oct 23, 2023

Conversation

cophus
Copy link
Member

@cophus cophus commented Aug 21, 2023

Both reduced and full PDFs added. Tested against simulated data. Still needs more work but basic functionality is there.

@bsavitzky
Copy link
Member

Thanks Colin! Starting a review now.

The first thing you'll want to do is pull from the dev branch (into your local version of this branch, then push to your remote fork you made the PR from) so that there aren't any merge conflicts in the PR. Happy to help if you run into any issues with that.

@bsavitzky
Copy link
Member

Also - could you attach some demo code with a (small) dataset that it runs on? It will make reviewing this easier :)

@cophus
Copy link
Member Author

cophus commented Aug 30, 2023

Here are the notebooks I used for testing - an (incomplete) abTEM simulation for amorphous Ta, plus a quick and dirty pair distribution function calculation:
image
the PDF calculation notebook produces:
image

Data is here:
https://drive.google.com/file/d/1g2d_uSTyVWr8zITLuVdzMVUkIPnLAvmL/view?usp=sharing

amor_Ta.zip

@bsavitzky bsavitzky self-requested a review October 16, 2023 23:01
@bsavitzky
Copy link
Member

Hey @cophus - I've made a number of changes, including

  • added documentation to all methods
  • modified input args to remove unused values, tweaked names
  • fixed return and plotting behaviors
  • fixed a .scattering_vectors bug - in the future use polardata.qq instead
  • fixed a bug where .radial_var_norm was assigned to radial_var rather than copied
  • added storage of and independent plotting methods for the outputs of the final calculate_pair_dist_function method
  • removed an extraneous call to .scattering_model

In the new doc strings for .calculate_radial_statistics and .calculate_pair_dist_function I've added specific descriptions of what these methods are computing. When you get a chance, please

  • review these equations for correctness, and
  • add citations to the docstrings for these two methods where noted ([@cophus TODO ADD CITATION])

@bsavitzky
Copy link
Member

Attached is an updated version of the .ipynb that runs on the code following my commits!

Once you've had a chance to check out these changes and make that edit, let me know and I can complete the review :)

RDF01.ipynb.zip

@bsavitzky
Copy link
Member

I'm merging this before the citations requested above have been added in order to have this module available in release code for a tutorial - the citations can get added in the next cycle...

@bsavitzky bsavitzky merged commit dc95f2d into py4dstem:dev Oct 23, 2023
6 checks passed
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.

2 participants