update nscf kpoints distance #446
Conversation
for more information, see https://pre-commit.ci
|
@unkcpz another solution should be that the value of k-points distance should change the protocol widget, if the user kept moderate, but uses a k-point distance lower or equal to 0.1, the protocol should be change to "precision" , though that changes the selection of pseudo |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #446 +/- ##
==========================================
+ Coverage 77.25% 77.49% +0.23%
==========================================
Files 43 44 +1
Lines 3082 3119 +37
==========================================
+ Hits 2381 2417 +36
- Misses 701 702 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
Why this is a problem? From what I understand, the nscf here is sort of independent, it uses the charge density from the scf calculation. So the nscf can be run at sparse k-points. In terms of the accuracy of the result, I also don't think it is a problem. I think we need use the protocol defined for pdos https://github.com/aiidateam/aiida-quantumespresso/blob/main/src/aiida_quantumespresso/workflows/protocols/pdos.yaml. Can you check if it is already the case? My opinion is that we should not add additional logic for the parameters in QeApp but more rely on the protocols of qe plugin. There are exceptions such as the magnetization structures/parameters that not yet properly deal with by qe plugin and we add the logic in the QeApp. |
|
@unkcpz I did test with some system in which i was defining a lower kpoint_distance , this affected the pdos, since it had less kpoints in the nscf |
|
For example if the user has protocol moderate, and the user modifies the kpoints_distance lower than the one of precise say 0.05 , the pdos (nscf) will be with 0.1 (because the protocol is moderate). So the override is modifying only the scf calculations accordingly but not taking into account the nscf. If you check the logic of the PR it respects the logic from the pdos protocol. |
|
Okay, I see your point. The kpoint_distance of nscf is not settable and always gets from protocol. The problem you encountered is the widget is provided to only override the kpoint_distance of scf calculation but not nscf. Ideally, for everything we/user suppose to change for builder, there should be a widget design for it. Do you agree with it? |
|
@unkcpz I am not sure. The issue is due to two factors the procotol and the kpoints_distance(override). What i propose is that if an user overrides the kpoints_distance the app should take into account two things.
|
|
I see. The issue comes from override and there is no logic between two kpoints_distance in Some thoughts and concerns below:
|
|
@unkcpz Thank you!, yes I agree (and would like) to discuss about it, to find the best solution |
no, this does not need the interaction between two plugins. Step 2 only provides the parameters, and it's the plugin developer decides how to use these two kpoint distances. In my option, if the user decides to override the kpoint distance, all the point distances (base, scf, nscf) should be overridden. So there is no need to compare with the protocol anymore. |
|
Thanks @superstar54
For this case yes, since one override (scf kpoint_distance) will be from advanced setting, it will be visible by all plugins. What if there are parameters that from plugin A, and plugin B might need it to decide the logic? Did you consider this scenario? How |
If a plugin developer still wants to add logic (dependence) between plugins. As far as I can foresee, there are three levels of logic,
|
|
Okay, makes sense to me. But it means we need to be careful about how to separate plugins, the band structure/ pdos probably need to be one panel called "electronic structure". This will also guide how properties plugin developers were to add the widgets or create a brand new plugin. |
|
Yes, a clear doc is needed. |
Hi @unkcpz! I'm not sure what solution you propose here. Do you want the |
Yes, that's what I thought. Do you have a follow-up discussion on how to implement override parameters with Lorenzo and Giovanni? My point is for the QeApp, if there are things that need more scientific logic, it better go to |
I'm going to be quite resistant to including any work chain logic that overrides parameters in the steps of the outline. I'm even opposed to setting defaults in the work chain, see aiidateam/aiida-quantumespresso#902. ^^ If I understand correctly, you are even proposing overriding inputs that are set in the work chain. It would be hard to convince me that this is a good idea.
Not really. Most work on that side is on hold until I am released from my scientific responsibilities. Happy to restart this once I move to PSI and have the time. 🙃 |
Nice, if we have unanimous agreement on the design principle, I am all good to stick with it. If I understand correctly, the aiida-quantumespresso is supposed to be made as flexible as possible to get even very "un-scientific" inputs and handle the errors.
May force be with you 🤌 Looking forward to seeing you here!! |
Exactly! Who are we to say what's reasonable? The work chain simply implements the logic, i.e. steps of the workflow. The protocol defined what we think is a reasonable set of inputs, but the user can change everything before submission. What the user sees in the builder is what the work chain will actually run with. And it is their responsibility to make sure the inputs are correct. The only caveats are:
|
|
Have this a second look, according to what @mbercx replied, the logic is better to add in here in QeApp. I'll rebase this after the advance setting is re-organized and get this merge. We don't need a further discussion I think. |
There was a problem hiding this comment.
Thanks @AndresOrtegaGuerrero! Had a quick look at the changes and left a comment with some questions.
| pw_overrides[key]["kpoints_distance"] = kpoints_distance | ||
|
|
||
| if protocol == "fast" and ( | ||
| kpoints_distance < 0.5 and kpoints_distance > 0.15 |
There was a problem hiding this comment.
Could you explain the logic of how the nscf kpoints_distance is set? The basic idea seems to be that if the user-specified override for kpoints_distance is smaller than the corresponding protocol value for the nscf, the value is set to that of the override. But for fast there is this kpoints_distance > 0.15 condition. Why is this? If e.g. the user sets protocol to fast and overrides the kpoints_distance to 0.1 the nscf kpoints should not be adjusted?
A couple of other notes:
- Maybe instead of hard-coding the protocol values for the
nscf, they can be extracted from the actual protocol? I thought we already did this elsewhere in the codebase. That way, the logic isn't broken when the protocol is changed inaiida-quantumespresso. - I'm wondering if it makes sense to set the
kpoints_distancefor thenscfto the same one as thescf. Isn't the point of thenscfto do a calculation of the electronic states at fixed charge density for a denser mesh to improve the precision? - Another option would be to also allow the user to specify the
kpoints_distanceoverride for thenscf, and e.g. the linearbandsdensity as well. This will of course further complicate the interface, and maybe we don't want to expose all these inputs to the user.
There was a problem hiding this comment.
@mbercx I should modify this logic it should be
if protocol == "fast" and kpoints_distance < 0.5
My idea is that is kpoint_distance is set less to 0.5 (protocol) then nscf should be the same the user is setting. (or less but i dont know at what percentage we should decrease)
|
@AndresOrtegaGuerrero not sure what happened, but it seems there are no longer any changes in this PR? |
|
@mbercx I am also puzzle , i just merged with the main, i will check |
|
@mbercx Now that the app is using plugin, the logic of override is done differently , from what I noticed the kpoints_distance in nscf is ignore. Am I wrong on this one @superstar54 ? |
|
@mbercx Now is updated ! (Sorry about that), I also removed the lines that didn't take into account the degauss and smearing in the nscf calculation. @superstar54 |
| @@ -64,7 +64,7 @@ pdos: | |||
| DOS: | |||
| DeltaE: 0.02 | |||
| nscf: | |||
| kpoints_distance: 0.1 | |||
| kpoints_distance: 0.12 | |||
There was a problem hiding this comment.
Why this change? What's this file, actually?
There was a problem hiding this comment.
this file is a yml to compare the builder generated from the app. This change is necessary since the nscf logic is being modify by this PR
| nscf_overrides["pw"]["parameters"]["SYSTEM"].pop("smearing", None) | ||
| nscf_overrides["pw"]["parameters"]["SYSTEM"].pop("degauss", None) | ||
| if protocol == "fast" and (kpoints_distance < 0.5): | ||
| nscf_overrides["kpoints_distance"] = kpoints_distance |
There was a problem hiding this comment.
As previously noted, I think approach is a little strange as it defeats the purpose of the NSCF (restarting from the charge density with a denser k-points mesh).
My vote would go to: if the user decides to override the k-points, they should have access to all densities.
As a side note (warning: incoming scope creep), I wonder how many users can think in terms of k-points distance. Maybe some guidelines or examples might be helpful, if not there already.
There was a problem hiding this comment.
So another approach could be to use a pdos Settings tab , and there the user can select the kpoints-distance for the nscf calculation. Regarding the k-points distance , the app already includes a widget that it displays the kpoint mesh that the calculation will be using
There was a problem hiding this comment.
I'd have to check the current interface (on my phone at the moment ^^), but either sounds fine by me.
|
@mbercx I re-implemented the PR, by using Setting Panel in the pdos pluggin |
| ) | ||
| # nscf kpoints setting widget | ||
| self.nscf_kpoints_distance = ipw.BoundedFloatText( | ||
| min=0.0, |
There was a problem hiding this comment.
| min=0.0, | |
| min=0.001, |
and remove the if else in lines 61-69.
| for trailet in trailets_list: | ||
| if hasattr(self.settings[identifier], trailet): | ||
| ipw.dlink( | ||
| (self.advanced_settings, trailet), | ||
| (self.settings[identifier], trailet), | ||
| ) |
There was a problem hiding this comment.
since the protocol is added here, please remove lines 79-84.
|
@AndresOrtegaGuerrero Thanks. very clear implementation. Two mirror changes are needed. I will approve it after your changes. |
|
|
||
| from aiidalab_qe.common.panel import Panel | ||
|
|
||
| # Periodicity |
There was a problem hiding this comment.
| # Periodicity | |
| # |
Please add a comment on why you use these map values.
When doing an override of the kpoints distance, the nscf calculation of PdosWorkChain is not updated. if the kpoint distance used is lower than the ones defined in the protocol, the scf calculation will have a higher number of kpoints , while the nscf will have the value of the protocol. This PR fix that behaviour