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

Align LDOS vectors to a reference #574

Merged
merged 10 commits into from
Oct 7, 2024

Conversation

timcallow
Copy link
Contributor

@timcallow timcallow commented Aug 27, 2024

Aligns a set of LDOS vectors to a reference. Optionally, the LDOS vectors can also be truncated from the left and right. This makes learning easier when the LDOS vectors become non-zero at different points, and/or are truncated (due to finite number of bands) at different points, such as when different mass densities are considered.

Remaining tasks need to be done or at least discussed before merging:

  • Parallelize over snapshots
  • Add alignment test
  • Extend to snapshots stored as openPMD
  • Include additional information files

@timcallow timcallow marked this pull request as ready for review September 4, 2024 13:53
@timcallow
Copy link
Contributor Author

Marking this as ready for review now. Some comments:

  • The example is not that informative since the LDOS vectors in the test data repo are aligned (by our definition) anyway. If something better is required, I could either (i) Create additional npy files in the test data (seems overkill for one, likely rarely-used, example) or (ii) create new data OTF in the script, which might make the example a bit convoluted and detract from what's actually going on. Perhaps an example is not even required and it should be converted to a test, in which case (ii) would be fine.
  • I haven't extended to openPMD because I'm not familiar with it. This problem is likely to be a recurrent one (also happened in PR Flexible snapshot number for data shuffling #570). Is the idea that all MALA developers understand how to manipulate openPMD data? Or one person (@RandomDefaultUser or @franzpoeschel) is responsible for implementing this functionality when required?
  • There is an optional number_of_electrons parameter which could be removed if the calculation output file is included. This tells the user how much the energy is shifted from the QE reference energy. I decided it was easier just to have the number_of_electrons parameter than have to set up an LDOS object using the calculation outputs, but can change if desired.

@franzpoeschel
Copy link
Contributor

I'll take a stab at it once I find the time. Feel free to merge before that, this can also go in separately.

@RandomDefaultUser
Copy link
Member

Thanks for the work @timcallow , looks good to me. I will merge this for now and open an issue to track the OpenPMD progress.

@RandomDefaultUser RandomDefaultUser merged commit 7a36d31 into mala-project:develop Oct 7, 2024
5 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.

3 participants