-
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
Mig/2.1 #271
Conversation
Since it is able to install the qe code and sssp library by |
for more information, see https://pre-commit.ci
@unkcpz FYI by turning on the deprecation warnings by setting btw: AiiDA v2.1.2 has been released yesterday. |
@danielhollas thanks for the advice. But how to turn on the config exactly, are the warning messages will show on the notebook? But the shell environment variables only have an effect on the shell that trigger it, correct? |
Yes, it is a bit tricky. Since I am building my own Docker image, I have simply set the environment variable in the Dockerfile. And yes, the warnings show up in the notebook UI. They might get swallowed in the Appmode, I don't remember. |
Seems it can be set for the notebook by ipython magic https://stackoverflow.com/questions/37890898/how-to-set-env-variable-in-jupyter-notebook. I'll give it a try tomorrow. I wonder if maybe there is AiiDA API to trigger this (AIIDA_WARN_v3=1)? @sphuber |
It is just an environment variable, so I guess you could do import os
os.env['AIIDA_WARN_v3'] = True |
Hi @superstar54, after the release of aiida-quantumespresso, I think there is no block for testing with this branch, can you test and review it again? Thanks! |
qe.ipynb
Outdated
"metadata": { | ||
"scrolled": false | ||
}, | ||
"outputs": [], | ||
"outputs": [ |
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.
Clean up this nb file.
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.
@unkcpz FYI I am running nbstripout
to get rid of these outputs as a pre-commit hook, see
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.
Interesting, thanks I'll give it a try.
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 PR by running a few examples with Si (diamond) structure.
- scf only
- scf + bands + pdos
- relax
- relax + bands + pdos
All the tests passed. Great!
There are two points which could be improved in the feature.
- After the calculation, I need to refresh the page to load the results (structure and bands).
- the pdos above the fermi level shift a little bit away from the axis, as shown below:
We have updated the widget-bandsplot, but in another PR. @dou-du and I make a lot changes of to the widget, so maybe the problem solved naturally. We can test it there. |
@unkcpz I think we can merge this PR after you clean up the nb file. The test only covers step 1 (structure selection). However, since I did all tests manually, I think it should be fine. In the future, tests on steps 2~4 should also be added. |
Totally agree! |
7b2b3ed
to
e1530d7
Compare
I open the issue in #316 The failed test is fixed. Somehow the firefox test failed because firefox is not installed properly. Since @superstar54 already mentioned the PR is ready. I merge it without approval. |
Add - uses: browser-actions/setup-geckodriver@latest test
- uses: browser-actions/setup-firefox@latest | ||
with: | ||
firefox-version: ${{ matrix.firefox }} | ||
run: which firefox |
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.
@unkcpz I think this line broke this whole file and tests are not running.
https://github.com/aiidalab/aiidalab-qe/actions/runs/3527890535
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.
You are right, I simply see All test pass. My fault, will open a PR to fix it.
fixes #272