Do not add auxiliary cell to uploaded/generated gas phase structures #592
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #592 +/- ##
==========================================
+ Coverage 85.06% 85.09% +0.02%
==========================================
Files 18 18
Lines 3388 3394 +6
==========================================
+ Hits 2882 2888 +6
Misses 506 506
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
danielhollas
force-pushed
the
remove-auxiliary-cell
branch
from
f70accc to
bf63f0d
Compare
danielhollas
force-pushed
the
remove-auxiliary-cell
branch
from
bf63f0d to
bb775ff
Compare
danielhollas
changed the title
|
@yakutovicha @cpignedoli can you have a brief look to see if you're okay with this PR? Since it is a (technically) breaking change, I'd like to get this in for v3 of AWB. I plan to add some unit tests here but would like to get a green light from you first. Thanks! |
|
Hi @danielhollas, I tested it, and it works. I don't have an issue with the principle, but if we leave it as is, it will create problems for all users currently using the QE app and the Empa apps for molecules. I would suggest triggering a warning for systems without a cell, and possibly adding a button to automatically create a cell using the logic that was enforced before. What do you think? |
danielhollas
force-pushed
the
remove-auxiliary-cell
branch
from
bb775ff to
862a4a0
Compare
This reverts commit 862a4a0.
|
@cpignedoli thanks. In that case, I think a better approach is to introduce an extra parameter in the widget initialized (e.g. Would that work? (please see the updated code). |
danielhollas
force-pushed
the
remove-auxiliary-cell
branch
from
129d05a to
8163c8b
Compare
…eUploadWidget If False, no auxiliary cell is added to gas phase molecules. True by default for backwards compatibility reasons.
danielhollas
force-pushed
the
remove-auxiliary-cell
branch
from
8163c8b to
55f2296
Compare
danielhollas
force-pushed
the
remove-auxiliary-cell
branch
from
49bd872 to
9b391b3
Compare
| title="", | ||
| description="Upload Structure", | ||
| allow_trajectories=False, | ||
| add_auxiliary_cell=True, |
There was a problem hiding this comment.
Not sure about the parameter name, suggestions welcome, especially shorter ones :-)
There was a problem hiding this comment.
To me, the name is good. It does the job.
|
I added unit tests and updated the PR description so this is ready for review. |
There was a problem hiding this comment.
The code looks good, I've also tested it on the new image with the migrated surfaces app, and it works without issues!
|
Perfect, thanks! |
3 participants
StructureUploadandSmilesWidgetcurrently automatically add a 10 Angstrom auxiliary cells to gas phase molecules. That's needed for plane wave codes, but is confusing and distracting for Quantum Chemistry codes such as ORCA.This PR introduces a new parameter (
add_auxiliary_cell) to these widgets to control this behaviour. The default is keptTruefor backwards compatibility (even though as a chemist (not material scientist) I am not happy about it :-D )