-
Notifications
You must be signed in to change notification settings - Fork 21
switch on the -pd
option for pw code
#1025
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
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1025 +/- ##
=======================================
Coverage 68.86% 68.87%
=======================================
Files 115 115
Lines 6415 6474 +59
=======================================
+ Hits 4418 4459 +41
- Misses 1997 2015 +18
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:
|
@@ -33,12 +34,18 @@ def update_resources(builder, codes): | |||
if "bands" in builder: | |||
set_component_resources(builder.bands.scf.pw, codes.get("pw")) | |||
set_component_resources(builder.bands.bands.pw, codes.get("pw")) | |||
builder.bands.scf.pw.settings = orm.Dict({"CMDLINE": ["-pd", ".true."]}) |
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.
should it be worth it to create a function in utils like use pencil_decomposition ? like that the code is not repeated , but also is easy to read in the future
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.
Good point, I will add a 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.
perhaps also this should be control in the PwCode model ? so the user can set and unset this ?
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 are two ways to handle this:
- as you suggested, add a setting in the GUI so that the
advanced
user can control it. - in the WorkChain, it handles the setting automatically or adds an error_handler to restart with a different number of CPUs.
I suggest discussing which way we want to implement in the future PR.
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.
yeah , i think we can leave it here, but in another PR we just leave the freedom plus the explanation to the user
56b5983
to
5391f74
Compare
5391f74
to
c863411
Compare
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.
LGTM!, thank you @superstar54 , I would suggest you open an issue, for implementing this in the resource panel in such a way the users can activate or deactivate this. Plus adding explanation to the users about resources and the computers they are using
Thanks @AndresOrtegaGuerrero , I opened an issue: #1031 |
fix #1021
Enable pencil decomposition for all
pw.x
calculations.Note, I still get issue on
projwfc
,