-
Notifications
You must be signed in to change notification settings - Fork 14
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
Running PDOS workchain and plot bands + pdos #159
Conversation
@unkcpz |
For the band structure output view widget tab name, it is only a good to have feature. So I think this PR is ready to review. |
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 @unkcpz! Just left some minor comments. Two other things:
- The dependency version of
aiida-quantumespresso
doesn't seem to be upgraded to3.5.0
? - When I refresh the app, the
dos.x
andprojwfc.x
codes weren't automatically set to the defaults. But this is most likely because I didn't install the app with this branch, since if I understand correctly the default are set upon installation.
@mbercx Thanks a lot for the review.
Ah, true. I forget to push the latest commits with some other changes.
This is an issue, I think the dos.x and projwfc.x were installed with pw.x. Just not sure which line of code influence this default fresh and setting behavior. I need to check this more. |
I make all the changes you mentioned in the review, but I need to give it a final run and test. Will request another review after it. |
I am not sure since I create a new container with install the app with this branch but got the QE setup error Moreover, I can not figure out how to retrigger the codes install and code setup or maybe just manually set the related parameters for code |
The only block of this is the code selector not setting the default code as expected. |
I will try to reproduce the issue. |
I think we should have a bot that automatically generates the following command for us 😁 : aiidalab install quantum-espresso@git+https://github.com/unkcpz/aiidalab-qe@pdos --yes |
I am still investigating, but I will start to try to answer some of your questions.
I just installed this app version in a new environment and did not experience any issues with the qe installation. However, I also found that the codes were not selected by default.
I am not able to reproduce this problem.
Indeed, I am not exactly sure how that currently works either. I will keep investigating.
To re-trigger the setup process just remove the AiiDA codes with the corresponding labels (pw-6.7@localhost etc.) and optionally also remove the Edit: You also have to remove the |
I can confirm that the pw.x code is currently not automatically selected, not even on the "master" branch. Trying to figure out when and how this regression was introduced. I have reported the issue here: #164 |
Please rebase this branch after merging #167 . |
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.
Please apply my suggestion on how to fix the remaining code selection issue and then rebase on master after #167 has been merged.
aiidalab_qe/steps.py
Outdated
self.codes_selector.pw.selected_code = _load_code(parameters["pw_code"]) | ||
self.codes_selector.dos.selected_code = _load_code(parameters["dos_code"]) | ||
self.codes_selector.projwfc.selected_code = _load_code( | ||
parameters["projwfc_code"] | ||
) |
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 would recommend to refactor this into a loop and keep the catch of the NotExistent
exception for robustness:
for code in ("pw_code", "dos_code", ...):
try:
...
except NotExistent:
pass
81a6161
to
984cba0
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.
I tested this on the latest aiidalab version and everything appeared to work fine, however the DOS and the Projwfc calculations never appeared to fully finish:
That's despite the fact the logs appear completed:
I have one last question regarding the widgets base requirement, but other than that this is good to go.
Add features to submit widget and output view widget to support running PdosWorkChain and show the pdos results.
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.
Fixes #62
qe.ipynb
pdos
andprojwfc
code setting will break backward compatibility.aiida-quantumespresso
plugin otherwise will raisetetrahydro
setting error.show_band_structure
widget tab name change. When only one bands or pdos calculation run.