Support using customize pseudopotential #435

unkcpz · 2023-08-15T12:01:15Z

The use case comes from I need to test Li pseudopotential I generated and plot the bands/pdos.

unit test
override or not?
the kind can be "O1", but the cutoffs are not properly set from pseudo family. Check by SiO2.
Unify the element/kind of PseudoUploadWidget

codecov · 2023-08-15T12:06:00Z

Codecov Report

Patch coverage: 88.01% and project coverage change: +3.10% 🎉

Comparison is base (3906b09) 53.42% compared to head (dabd8fa) 56.52%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #435      +/-   ##
==========================================
+ Coverage   53.42%   56.52%   +3.10%     
==========================================
  Files          26       26              
  Lines        2104     2321     +217     
==========================================
+ Hits         1124     1312     +188     
- Misses        980     1009      +29

Flag	Coverage Δ
python-3.10	`56.52% <88.01%> (+3.10%)`	⬆️
python-3.8	`56.57% <88.01%> (+3.10%)`	⬆️
python-3.9	`56.57% <88.01%> (+3.10%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
src/aiidalab_qe/app/configuration/__init__.py	`36.14% <40.00%> (-0.57%)`	⬇️
src/aiidalab_qe/app/submission/__init__.py	`70.60% <50.00%> (-0.78%)`	⬇️
src/aiidalab_qe/app/configuration/pseudos.py	`87.67% <86.07%> (-5.18%)`	⬇️
tests/conftest.py	`97.82% <100.00%> (+0.22%)`	⬆️
tests/test_pseudo.py	`100.00% <100.00%> (ø)`
tests/test_submit_qe_workchain.py	`93.50% <100.00%> (+0.35%)`	⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

unkcpz · 2023-08-16T16:31:55Z

@AndresOrtegaGuerrero this new widget borrow some idea from your magnetization setting widget. Let's see if we can converge them into a generic widget for setting things that dynamically changed with structure and depend on structure kinds.

AndresOrtegaGuerrero · 2023-08-17T07:41:59Z

@unkcpz I really like the idea, specially because I was doing a test with PseudoDojo using the cutoff from the app, and it seems the cutoff I obtained were not well converged. So it would be nice if it also appears what is the overall wfc and rho cutoff use for the simulation. In case the user wants to do a convergence test (or just increase the cutoff)

AndresOrtegaGuerrero · 2023-08-17T08:28:48Z

@unkcpz I think you should add like a caption that explains the user they can upload and/or modify each Pseudo

unkcpz · 2023-08-17T08:51:09Z

I think you should add like a caption that explains the user they can upload and/or modify each Pseudo

Good point, the explanation on how the cutoffs for the pw calculation are get from cutoffs of pseudos should also explain.

aiidalab_qe/app/steps.py

AndresOrtegaGuerrero · 2023-08-17T08:53:25Z

@AndresOrtegaGuerrero this new widget borrow some idea from your magnetization setting widget. Let's see if we can converge them into a generic widget for setting things that dynamically changed with structure and depend on structure kinds.

Yes maybe we can make a widget like DynamicalSettings

unkcpz · 2023-08-18T09:34:28Z

Hi @AndresOrtegaGuerrero, can you give this a second look? It is already from my side. For making it generic, I'll open an issue for future implementation, I notice @mikibonacci also mention it in the comment of plugin UI pr. It is certainly worth doing.

AndresOrtegaGuerrero · 2023-08-18T09:59:36Z

@unkcpz thank you!, I just check it looks good for me. My opinion will be that you should include two widgets to define the wfc and rho cutoff (in case the ones defined is not enough). I am saying out of experience, i tried to run something with the app using PseudoDojo, to later realized that the cutoffs were really off and not converged. Then to be forced to stop using the App since i didn't have control over it. I think if you are already modifying the pseudos you should include this.

unkcpz · 2023-08-18T11:28:53Z

My opinion will be that you should include two widgets to define the wfc and rho cutoff (in case the ones defined is not enough)

Ah! I see your point. Actually, you can just set the ecutwfc/ecutrho higher for one pseudo and it will be used. But you are right, this looks like a non-standard trick. I'll try to implement that.

unkcpz · 2023-08-18T11:43:05Z

Emm.... Right after I start, I feel it does not fully make sense to have one more widget to set it. An extra cutoff widget are a but redundant as the one for setting pseudos cutoffs separately.
It looks not bad for changing the cutoffs for pseudos from pseudo families. But in my case, I upload and use my own pseudo, and the ecutwfc/ecutrho are just showing 0 and it surely encourages the user to set it.
Can we keep it as this, and I'll remember to bring this layout design to Nicola and ask for feedback? (there are more such design decisions, I'll make an issue for this.)

AndresOrtegaGuerrero · 2023-08-18T12:32:13Z

@unkcpz I would suggest that for the design, there should be only control over ecutwfc and ecutrho (of the calculation). The user can upload a different functional per element or kind defined. I dont think there is need to display or control the cutoff of each element. In particular if the user is going to combine US , PAW or NC pseudos. The user rather have control over the ecutwfc and ecutrho of the calculation.

unkcpz · 2023-08-18T12:44:31Z

I dont think there is need to display or control the cutoff of each element.

I agree. Then it'll be after the pseudo, I display the cutoffs, if it is customized pseudo, I show "No recommended ecutwfc".
The FloatText input widget is provided for the user to change. I'll do the change.

In particular if the user is going to combine US , PAW or NC pseudos.

Why you think this is a special case? In my SSSP verification, US/PAW is used, I use dual=8 and for NC use dual=4 and run convergence. Do you mean sometimes higher cutoffs are required?

AndresOrtegaGuerrero · 2023-08-18T13:02:07Z

I dont think there is need to display or control the cutoff of each element.

I agree. Then it'll be after the pseudo, I display the cutoffs, if it is customized pseudo, I show "No recommended ecutwfc". The FloatText input widget is provided for the user to change. I'll do the change.

In particular if the user is going to combine US , PAW or NC pseudos.

Why you think this is a special case? In my SSSP verification, US/PAW is used, I use dual=8 and for NC use dual=4 and run convergence. Do you mean sometimes higher cutoffs are required?

What did you mean by dual? You meant the ration between ecutwfc and ecutrho, like in SSSP is ecutrho = (8 to 12) * ecutrho and in NC is ecutrho = 4* ecutwfc ?

unkcpz · 2023-08-18T13:40:22Z

You meant the ration between ecutwfc and ecutrho, like in SSSP is ecutrho = (8 to 12) * ecutrho and in NC is ecutrho = 4* ecutwfc ?

Yep. But anyway, I think we have a good plan for how to improve it.

for more information, see https://pre-commit.ci

…ument

AndresOrtegaGuerrero · 2023-08-18T14:32:57Z

@unkcpz Yes, you can have like only two widget to control ecutwfc and ecutrho. once you select the pseudopotential the value of these two widgets get set by the values given by aiida-pseudo. Then the user can decide to increase them accordingly. In a future release, once the plugin PR is merge, we can even make one tab to run a convergence workflow when the user can select a list of ecutwfc to test.

unkcpz · 2023-08-18T16:20:40Z

Hi @AndresOrtegaGuerrero, I did the change, please have another look.

AndresOrtegaGuerrero · 2023-08-21T08:07:10Z

@unkcpz Thank you! , It looks great to me. I did a few test didn't find any issue. One suggestion I would add is that if would be good that the cutoffs wfn and rho, should be included in the override of pseudos

unkcpz · 2023-08-21T13:57:52Z

src/aiidalab_qe/app/submission/__init__.py

+                pw_overrides[key].setdefault("pw", {"parameters": {"SYSTEM": {}}})
+                pw_overrides[key]["pw"]["parameters"]["SYSTEM"][
+                    "ecutwfc"
+                ] = self.pseudo_setter.ecutwfc
+                pw_overrides[key]["pw"]["parameters"]["SYSTEM"][
+                    "ecutrho"
+                ] = self.pseudo_setter.ecutrho


See here how ecutwfc/ecutrho in pw_overrides affect the bulider.

Yes, that i tested. What i meant was in the GUI, that the widget is not available if the checkbox is activated

I see~ I don't want to make this change for now. 1. It may not be proper to control the cutoffs setup with pseudo override. 2. we need more discussion on what needs to be overridden, how to display the override checkbox.
If we follow the other parameters that exist or are added by you, there in principle should be a checkbox for every parameter. But maybe that is not necessary. @superstar54 was suggesting since the user on this page would anyway want/know how to change things, maybe we don't need checkboxes.

Let's discuss this on the issue #444

unkcpz · 2023-08-21T13:57:57Z

One suggestion I would add is that if would be good that the cutoffs wfn and rho, should be included in the override of pseudos

Not quite sure what you mean. It is passed to the builder through the override and it will change the summary report when it is changed.

AndresOrtegaGuerrero

Look good to me

unkcpz marked this pull request as draft August 15, 2023 12:01

unkcpz force-pushed the fix/201/customize-pseudo branch 3 times, most recently from 9b63c35 to 0f748b1 Compare August 15, 2023 23:15

unkcpz marked this pull request as ready for review August 16, 2023 16:29

unkcpz requested review from AndresOrtegaGuerrero and superstar54 August 16, 2023 16:29

AndresOrtegaGuerrero reviewed Aug 17, 2023

View reviewed changes

aiidalab_qe/app/steps.py Outdated Show resolved Hide resolved

unkcpz requested a review from AndresOrtegaGuerrero August 17, 2023 10:45

unkcpz mentioned this pull request Aug 18, 2023

Layout/ text / intuitive widget design: feedback requirement #440

Open

2 tasks

unkcpz force-pushed the fix/201/customize-pseudo branch 3 times, most recently from 9c6b954 to 1161f09 Compare August 18, 2023 13:52

unkcpz mentioned this pull request Aug 18, 2023

Adapt modules and files layout for plugin implementation #439

Merged

unkcpz force-pushed the fix/201/customize-pseudo branch from 1161f09 to 3fb264b Compare August 18, 2023 13:57

Add test case to support showing pseudo list

79bc325

unkcpz and others added 13 commits August 18, 2023 13:58

Add test for the pseudo setter

3f84450

workable version of PseudoSetter

f1f84da

Add error messege and check the element conform

51242be

use PseudoSetter

2572f62

Finize the PseudoSetter

4d6b353

[pre-commit.ci] auto fixes from pre-commit.com hooks

800a58a

for more information, see https://pre-commit.ci

f-m: kind and pseudos

7e768bf

Test for pseudo uploader widget

1035ac7

Change structure_data_object to generate_strctrue_data and accept arg…

9f5ff6c

…ument

Test for pseudo setter

ada674f

message show of cutoffs and helper

6c21a56

Strucute not set info red text

90a2cb6

fx after rebase

6e483a8

unkcpz force-pushed the fix/201/customize-pseudo branch from 3fb264b to 6e483a8 Compare August 18, 2023 13:58

unkcpz added 4 commits August 18, 2023 15:51

f-a: Set ecutwfc/ecutrho directly

acffb17

fix problem of unit of dojo library

bb70ed6

f-m: setter

01257c2

fix test

dabd8fa

unkcpz commented Aug 21, 2023

View reviewed changes

AndresOrtegaGuerrero approved these changes Aug 22, 2023

View reviewed changes

unkcpz merged commit bd7f39b into main Aug 22, 2023
13 checks passed

unkcpz deleted the fix/201/customize-pseudo branch August 22, 2023 09:16

unkcpz mentioned this pull request Sep 26, 2023

Allow to use other pseudopotentials #201

Closed

2 tasks

superstar54 mentioned this pull request Oct 17, 2023

Summary report is wrong when a user uploads custom pseudos. #511

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support using customize pseudopotential #435

Support using customize pseudopotential #435

unkcpz commented Aug 15, 2023 •

edited

Loading

codecov bot commented Aug 15, 2023 •

edited

Loading

unkcpz commented Aug 16, 2023

AndresOrtegaGuerrero commented Aug 17, 2023 •

edited

Loading

AndresOrtegaGuerrero commented Aug 17, 2023

unkcpz commented Aug 17, 2023

AndresOrtegaGuerrero commented Aug 17, 2023

unkcpz commented Aug 18, 2023

AndresOrtegaGuerrero commented Aug 18, 2023

unkcpz commented Aug 18, 2023

unkcpz commented Aug 18, 2023

AndresOrtegaGuerrero commented Aug 18, 2023 •

edited

Loading

unkcpz commented Aug 18, 2023 •

edited

Loading

AndresOrtegaGuerrero commented Aug 18, 2023

unkcpz commented Aug 18, 2023

AndresOrtegaGuerrero commented Aug 18, 2023

unkcpz commented Aug 18, 2023

AndresOrtegaGuerrero commented Aug 21, 2023 •

edited

Loading

unkcpz Aug 21, 2023

AndresOrtegaGuerrero Aug 21, 2023

unkcpz Aug 22, 2023

unkcpz Aug 22, 2023

unkcpz commented Aug 21, 2023

AndresOrtegaGuerrero left a comment

Support using customize pseudopotential #435

Support using customize pseudopotential #435

Conversation

unkcpz commented Aug 15, 2023 • edited Loading

codecov bot commented Aug 15, 2023 • edited Loading

Codecov Report

unkcpz commented Aug 16, 2023

AndresOrtegaGuerrero commented Aug 17, 2023 • edited Loading

AndresOrtegaGuerrero commented Aug 17, 2023

unkcpz commented Aug 17, 2023

AndresOrtegaGuerrero commented Aug 17, 2023

unkcpz commented Aug 18, 2023

AndresOrtegaGuerrero commented Aug 18, 2023

unkcpz commented Aug 18, 2023

unkcpz commented Aug 18, 2023

AndresOrtegaGuerrero commented Aug 18, 2023 • edited Loading

unkcpz commented Aug 18, 2023 • edited Loading

AndresOrtegaGuerrero commented Aug 18, 2023

unkcpz commented Aug 18, 2023

AndresOrtegaGuerrero commented Aug 18, 2023

unkcpz commented Aug 18, 2023

AndresOrtegaGuerrero commented Aug 21, 2023 • edited Loading

unkcpz Aug 21, 2023

Choose a reason for hiding this comment

AndresOrtegaGuerrero Aug 21, 2023

Choose a reason for hiding this comment

unkcpz Aug 22, 2023

Choose a reason for hiding this comment

unkcpz Aug 22, 2023

Choose a reason for hiding this comment

unkcpz commented Aug 21, 2023

AndresOrtegaGuerrero left a comment

Choose a reason for hiding this comment

unkcpz commented Aug 15, 2023 •

edited

Loading

codecov bot commented Aug 15, 2023 •

edited

Loading

AndresOrtegaGuerrero commented Aug 17, 2023 •

edited

Loading

AndresOrtegaGuerrero commented Aug 18, 2023 •

edited

Loading

unkcpz commented Aug 18, 2023 •

edited

Loading

AndresOrtegaGuerrero commented Aug 21, 2023 •

edited

Loading