Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add support for having non-displaced atoms in Phonopy routines #2110
base: main
Are you sure you want to change the base?
Add support for having non-displaced atoms in Phonopy routines #2110
Changes from 13 commits
d2aa51a
24608c9
d6deffc
1f31546
a7b3a0c
38a4230
7430b8a
6939b90
54c4e5f
b7789d2
5536691
93a66a6
75b8f9f
56fc1ac
b98984e
15f4469
e715946
f39bf76
2225c9d
0d06540
8817af3
923ca89
2f3e943
1d710db
7329f9b
e102d40
98d0274
4f1027e
0dec274
5f99ce4
b87a8f7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be ideal to have a simple unit test for this specific function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no
min_lengths
in this function. I think we can just ignore that part. Also, it must be specified since it is a positional argument.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing a line break before
Returns
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to be unnecessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is relevant anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was my point of confusion. I had thought you were implying that additional atoms would be for the adsorbates (I now see that's not the case). In reality, it would be for the surface of course.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I completely see where this confusion can get from, this is why I am not entirely sure about the current name.
Although this forces the users to stop and think before using this important approximation
EDIT: yes, the doc does not help as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One downside of
fixed_atoms
is that it potentially implies that they are sites withinatoms
(the first positional argument) that are fixed, whereas in reality this is a separateAtoms
object. That's one reason why I likeadditional_atoms
since it makes it clear that this is a separateAtoms
object, although one would understand that if they read the type hint. Of course,additional_atoms
doesn't inherently specify anything about them being fixed until you read the docstring.I think either could be okay, personally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume you just removed the type hints for brevity's sake since they are basically implied elsewhere? I was also thinking about this since it is an internal function. Although, if we at some point enabled static type checking, having the type hints would ensure there isn't an error. I doubt we will be able to get to the point where we enable static type checking though... certainly not anytime soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a little bit problematic, another option is to send both displaced and non_displaced and optimise them and separate them again...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. I mean, I guess we can ask if we really even need a relaxation step beforehand? Presumably one could just call
relax_job()
before calling the flow and it would be exactly the same (although without a tight force tolerance).If there's a desire to keep this though, then yeah I think the only route would be to relax
displaced_atoms+non_displaced_atoms
as a singlecombined_atoms
and then pass incombined_atoms[:len(displaced_atoms)]
etc. tophonon_subflow
. It does, admittedly, complicate things a bit because it seems weird to relax something that is called "non-displaced", but... 🤷There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this test still needs updating.
get_phonopy()
doesn't return a tuple anymore, and we aren't dealing withFixAtoms
. That's the reason for the failure in the CI.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the test failure in CI, it looks like the original
atoms
object is getting mutated somewhere (since it has an EMT calculator attached)? That doesn't seem ideal... 😅There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this was related to #2230.