Add support for having non-displaced atoms in Phonopy routines

[tomdemeyere]

Summary of Changes

Should be much better

Checklist

• [ ] I have read the "Guidelines" section of the contributing guide. Don't lie! 😉
• [ ] My PR is on a custom branch and is not named main.
• [ ] I have added relevant, comprehensive unit tests.

Notes

• Your PR will likely not be merged without proper and thorough tests.
• If you are an external contributor, you will see a comment from @buildbot-princeton. This is solely for the maintainers.
• When your code is ready for review, ping one of the active maintainers.

[buildbot-princeton]

Can one of the admins verify this patch?

[tomdemeyere]

Overall additional atoms can be seen as the same to "fixed_atoms". I renamed it to put an emphasis on the fact that these atoms are merely spectators of the Phonons calculation, only having an implicit influence through the forces. When you think about this has nothing to do with "fixing".

If you want to calculate an adsorbate on a slab that way you would just send the adsorbate part to regular atoms and the slab as "additional_atoms".

This could be renamed as "fixed_atoms" as well, I am trying to explore the semantic here

[Andrew-S-Rosen]

Overall additional atoms can be seen as the same to "fixed_atoms". I renamed it to put an emphasis on the fact that these atoms are merely spectators of the Phonons calculation, only having an implicit influence through the forces. When you think about this has nothing to do with "fixing".

If you want to calculate an adsorbate on a slab that way you would just send the adsorbate part to regular atoms and the slab as "additional_atoms".

This could be renamed as "fixed_atoms" as well, I am trying to explore the semantic here

To clarify, I understand that part. My question was about understanding why and when this is relevant, given that it is the opposite from what is usually invoked in the literature.

Edit: Ah, you are saying the slab would go to additional atoms. Sure, that makes sense. I think that conflicts with what you wrote in one of the docstrings. I think additional_atoms makes sense.

[Andrew-S-Rosen]

Historically, in such scenarios, I have used ASE's Vibrations module (specifying the atoms to vibrate with indices) and then passed that information into ASE's HarmonicThermo class. However, using phonopy for this seems like it might be even better because it would be possible to run the displacement calculations in parallel rather than sequentially.

At some point, I was going to add HarmonicThermo to quacc.runners.thermochemistry to support this, but the phonopy approach is great.

[Andrew-S-Rosen]

What if we rename the first positional argument to unfixed_atoms and then additional_atoms as fixed_atoms? Not sure if that helps or hurts...

[tomdemeyere]

Claude 3 Opus is suggesting (😅):

1. primary_atoms (instead of atoms) and auxiliary_atoms
2. atoms and fixed_atoms
3. displaced_atoms (instead of atoms) and nondisplaced_atoms

In this order of "preference", I might actually lean towards 2, (let's just be pragmatic?)

EDIT: To be fair, 3 looks like your last suggestion but without the double negative of "unfixed", this might be the best option to clearly distinguish between what is being moved and the rest, "nondisplaced_atoms" might just be "fixed_atoms" for simplicity?

[Andrew-S-Rosen]

Ooh I like 3! It's better than my suggestion because it doesn't accidentally imply a FixAtoms constraint. But I'm okay with 2 if you have a strong opinion about it.

[tomdemeyere]

That's a good argument let's go with that, I just added an extra underscore

[tomdemeyere]

@Andrew-S-Rosen

I did not forget this PR, as you may know I am very busy currently, I will definitely come back to it ASAP!

[Andrew-S-Rosen]

There is no rush at all :) Good luck with wrapping up the PhD!

[tomdemeyere]

I have some more time to do what I like now :)

I added some of your suggestions, namely:

• The type hinting in the internal _thermo_job
• Fixed the unnecessary comments
• Added a test for the get_phonopy_supercell

You will see that I added the non_displaced_atoms object to additional_fields in the phonopy_subflow. I did it because otherwise the result dict does not contain any information about these atoms at all. There might be a better way to do this? I will let you decide.

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 97.42%. Comparing base (7fef05b) to head (3c9bb51).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2110      +/-   ##
==========================================
+ Coverage   97.41%   97.42%   +0.01%     
==========================================
  Files          85       85              
  Lines        3518     3536      +18     
==========================================
+ Hits         3427     3445      +18     
  Misses         91       91              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[Andrew-S-Rosen]

@tomdemeyere: Sorry about the back-and-forth. Could you remind me the motivation to switch from specifying a list[int] to two Atoms objects?

Otherwise, once the type hint is fixed, I'll merge pending resolution of the above discussion.

[tomdemeyere]

@Andrew-S-Rosen I don't really remember, this might be linked to other problems at the time.

It seems that changing to the index approach is rather straightforward, and might make more sense?

[Andrew-S-Rosen]

Let's give it a go and see what happens! It seems a tad easier on the user in my opinion. Since it should also support negative indexing, that should make things pretty trivial for the user since it's usually the last N indices where an adsorbate is.

[tomdemeyere]

@Andrew-S-Rosen Tell me what you think.

I also made sure that the Schema returns the same Atoms object as the one being sent. I also included non_displaced_atoms and displaced_atoms when relevant.

[Andrew-S-Rosen]

Thanks! This looks great! Happy to merge!