-
Notifications
You must be signed in to change notification settings - Fork 15
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
adding ProjwfcBandWorkchain #787
Conversation
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
This PR introduces the Fat bands feature to the app. #826 #819 I propose to do a new workchain BandsWorkChain, to select between PwBands vs ProjwfcBands, I was wondering if we can debate about this , first if you guys agree and/or if you have other suggestions. @superstar54 @mikibonacci , @edan-bainglass @superstar54 (I was wondering if you could help me the pytest failing? |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #787 +/- ##
==========================================
- Coverage 67.98% 67.95% -0.04%
==========================================
Files 49 50 +1
Lines 4389 4475 +86
==========================================
+ Hits 2984 3041 +57
- Misses 1405 1434 +29
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There is a new release of aiida-wannier90-workflows , so we can evaluate this PR for merging it with aiidalab-qe |
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.
Generally looks okay. See my comments, but no changes required, just clarification.
aiida.workflows = | ||
aiidalab_qe.bands_workchain = aiidalab_qe.plugins.bands.bands_workchain:BandsWorkChain | ||
|
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.
We already had bands calculation via the built-in bands plugin. I'm surprised there wasn't already an associated workflow.
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.
No , because , we were calling a workflow from aiida-quantumespresso plugin, therefore that workchain is already present. Here I did a new one to be able to switch between the from aiida-quantumespresso and the one from aiida-wannier90-workflows (capable of the fat bands)
@@ -0,0 +1,382 @@ | |||
import math |
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.
Conceptually, what's the difference between workchain.py and bands_workchain.py? The latter breaks from the plugin template a bit, no?
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, the workchain.py, is that one used within the "architecture" of the plugin system within aiidalab-qe. While this bands_workchain is the actual aiida workflow, that workchain.py (is more like a controller that calls the workflow builder) passes the builder for the app.
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.
Why not migrate this new workflow to aiida-quantumespresso
?
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.
Never mind, I should have read the comment above first 😅 So it is because it combines workflows from two plugins, correct?
if "bands" in self.node.outputs.bands: | ||
bands_node = self.node.outputs.bands.bands | ||
elif "bands_projwfc" in self.node.outputs.bands: | ||
bands_node = self.node.outputs.bands.bands_projwfc |
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.
So it's regular of fat? Am I understanding it correctly? Not possible to combine the mechanics? I guess it's a different code, so no.
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, the feature somehow are in different aiida plugins, the "regular" is from aiida-quantumespresso, while the "fat bands" from the wannier90-workflows
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.
Thanks for the PR @AndresOrtegaGuerrero, I added few comments on something that is unclear to me
], | ||
value="hexagonal", | ||
self.projwfc_bands = ipw.Checkbox( | ||
description="Fat bands calculation", |
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.
Hi @AndresOrtegaGuerrero, could you add some more description here? maybe is not clear what "fat bands" mean, for a non expert user (I am thinking about experimentalists). Thanks!
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 added a description let me know what you think
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.
Hi @AndresOrtegaGuerrero, for me it is good! thanks!
@@ -20,7 +20,10 @@ def _update_view(self): | |||
pdos_node = None | |||
|
|||
try: | |||
bands_node = self.node.outputs.bands | |||
if "bands" in self.node.outputs.bands: |
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.
Hi @AndresOrtegaGuerrero, I don't understand why we need to put this block of code, as it it already in plugins/bands/result.py
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 because now we have a BandWorkChain, that selects between the one from aiida-quantumespresso or the one from aiida-wannier90-workflows (for doing fat bands)
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. thanks!
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 will modify this code , because , if we merge it like this , it wont be compatible with previous versions that contain Bands calculations
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.
Hi @AndresOrtegaGuerrero, LGTM! We can merge the PR
@mikibonacci @edan-bainglass , I modify the result.py part from the bands plugin. With this change there is backwards compatibility of bands calculations using previous versions of the app. |
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! Thanks
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, thanks!
This PR includes the ProjwfcBandWorkChain from aiida-wannier90-workflows to add fat band calculation option in the Bands plugin