-
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
Feature/adv tot charge #402
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #402 +/- ##
==========================================
+ Coverage 50.53% 52.13% +1.60%
==========================================
Files 16 16
Lines 1789 1849 +60
==========================================
+ Hits 904 964 +60
Misses 885 885
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
@mbercx I was wondering if maybe you could give me a hint of what could be the sources in the failure of our test with python 3.9 and 3.8 ?? |
@AndresOrtegaGuerrero thanks for the ping! Yeah our nightly builds also started failing, not sure what's going on, but will prioritise fixing it! Very weird that even the released version |
Probably related to explosion/spaCy#12659 |
Was most likely due to a bug in aiidateam/aiida-quantumespresso#941 I reran some tests that were failing for an |
Thank you ! |
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.
@AndresOrtegaGuerrero thanks a lot!
I am wondering how we categorize parameters as "advanced". One option is anything beyond protocol is advanced. Or the smearing and kpoints overrides are categorized as advance setting as well.
And what confuses me most is that we already have the Advance Setting
tab but it has another advance setting. I think it is true we need a parent widget to collect all such "advance setting" widgets in one. Why not we put all the widgets in "Advance Settings" tab to a widget called AdvanceSettings
and manage the display there solely?
aiidalab_qe/app/steps.py
Outdated
@@ -1077,6 +1166,11 @@ def _extract_report_parameters( | |||
] = builder.bands.bands_kpoints_distance.value | |||
parameters["nscf_kpoints_distance"] = builder.pdos.nscf.kpoints_distance.value | |||
|
|||
if builder.relax.base["pw"]["parameters"]["SYSTEM"]["tot_charge"]: |
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.
if builder.relax.base["pw"]["parameters"]["SYSTEM"]["tot_charge"]: | |
tot_charge = builder.relax.base["pw"]["parameters"]["SYSTEM"]["tot_charge"] | |
if total_charge: |
This better move to above under the comment where said why the value getting from builder.relax.base
is feasible.
I also don't quite understand, the tot_charge
can be a None
?
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 it can be None
, if the user do not apply advanced settings override, or the tot_charge override
@@ -92,6 +92,10 @@ | |||
<td>{{ nscf_kpoints_distance }} Å<sup>-1</sup></td> | |||
</tr> | |||
{% endif %} | |||
{% if tot_charge %} |
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 don't think this logic is needed, if tot_charge not set, it is 0
by default, we can show it as well.
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.
Is needed, if the user doesnt activate the tot_charge , that value should be None. Thats why the widget has a general override, so the user can determine if advanced setting will be use, and then tot_charge ,( or any future option) should have also an override (to determine if you want to use it or not)
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.
if the user doesnt activate the tot_charge , that value should be None.
If tot_charge widget not activate, can it be set to 0.0
, then we don't need this if tot_charge
. We can always have tot_charge in pw input 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 The issue (i feel, not sure about it) is about compatibility (if we care at this point) , if calculations do not have the tot_charge in the parameters, an error will always be present to visualize the results. What do 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.
I removed the logic
@AndresOrtegaGuerrero thanks! Look at the failed test on py3.10, the change on total charge is expected but I am worried about the kpoints_distance also changing. Can you check if the widget worked as expected that the kpoints can be correctly overrided? To get the new regression tast data you just need to run |
…aiidalab-qe into feature/adv_tot_charge
@unkcpz I just updated the conftest |
@unkcpz Now it should be fine |
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.
@AndresOrtegaGuerrero Thanks again! Still, some minor requests, but the overall shape is very satisfactory.
aiidalab_qe/app/steps.py
Outdated
self.smearing_settings = SmearingSettings() | ||
self.kpoints_settings = KpointSettings() |
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 use self.advanced_settings.smearing_settings
multiple times. It is better to have an interface for the class. See comment above.
description = ipw.HTML("""Select the advanced settings for the <b>pw.x</b> code.""") | ||
# set here defaul values for the advanced settings | ||
tot_charge_default = 0 | ||
|
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.
smeaning_settings = tl.Instance(SmearingSettings()) | |
kpoints_settings = tl.Instance(KpointsSettings()) |
Add interfaces for the parent class.
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.
If I use this here i get an error, however the interface is establish with AdvancedSetting
in ConfigureQeAppWorkChainStep
and SubmitQeAppWorkChainStep
.
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.
What is the error you got exactly?
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.
if i use the traitlets.Instance , it complaints is not defined , However it is not needed since the traitlets is now on AdvancedSettings,
like it was for SmearingSettings and KpointsSettings in ConfigureQeAppWorkChainStep
and SubmitQeAppWorkChainStep
. So there shouldn't be an issue here
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.
Okay, no problem from my side. Thanks!
aiidalab_qe/app/steps.py
Outdated
"kpoints_distance_override" | ||
] | ||
self.kpoints_settings.override_protocol_kpoints.value = True | ||
self.advanced_settings.kpoints_settings.kpoints_distance.value = ( |
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.
These long attributes call really frightened me, same for the smearing_settings
. I think it makes sense to get rid of the duplications of variable names.
advanced_settings
andkpoints_settings
both has "settings", maybekpoints_settings
->kpoints
?kpoints_distance
is underkpoints_settings
, maybe "distance" is enough?
So this line can be self.advanced_settings.kpoints.distance.value
.
I am not very sure this won't bring more ambiguity, if it is too much, you can ignore this. Any idea?
@@ -92,6 +92,10 @@ | |||
<td>{{ nscf_kpoints_distance }} Å<sup>-1</sup></td> | |||
</tr> | |||
{% endif %} | |||
{% if tot_charge %} |
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.
if the user doesnt activate the tot_charge , that value should be None.
If tot_charge widget not activate, can it be set to 0.0
, then we don't need this if tot_charge
. We can always have tot_charge in pw input file.
…educe ambiguity (Jusong)
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.
Almost there, three tiny requests.
description = ipw.HTML("""Select the advanced settings for the <b>pw.x</b> code.""") | ||
# set here defaul values for the advanced settings | ||
tot_charge_default = 0 | ||
|
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.
What is the error you got exactly?
] = self.advanced_settings.smearing.smearing.value | ||
|
||
# smearing degauss setting | ||
pw_overrides[key]["pw"]["parameters"]["SYSTEM"][ |
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 you remove the setdefault
clause? Python dictionary method setdefault()
is similar to get(), but will set dict[key]=default if key is not already in dict.
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 Because if self.advanced_settings.override.value
can control if you are going to use overrides , and the next line already creates the dictionary pw_overrides[key]["pw"] = {"parameters": {"SYSTEM": {}}}
.
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, this is more clear, nice!
aiidalab_qe/app/steps.py
Outdated
@@ -1077,6 +1191,12 @@ def _extract_report_parameters( | |||
] = builder.bands.bands_kpoints_distance.value | |||
parameters["nscf_kpoints_distance"] = builder.pdos.nscf.kpoints_distance.value | |||
|
|||
tot_charge = builder.relax.base["pw"]["parameters"]["SYSTEM"]["tot_charge"] |
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.
It will fail if SYSTEM namelist don't have tot_charge
, correct?
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.
Yes, I will modify this
aiidalab_qe/app/steps.py
Outdated
tot_charge = builder.relax.base["pw"]["parameters"]["SYSTEM"]["tot_charge"] | ||
if tot_charge: | ||
parameters["tot_charge"] = tot_charge | ||
else: | ||
parameters["tot_charge"] = 0.0 | ||
|
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.
tot_charge = builder.relax.base["pw"]["parameters"]["SYSTEM"]["tot_charge"] | |
if tot_charge: | |
parameters["tot_charge"] = tot_charge | |
else: | |
parameters["tot_charge"] = 0.0 | |
parameters["tot_charge"] = builder.relax.base["pw"]["parameters"]["SYSTEM"].get("tot_charge", 0.0) |
And probably better having a _DEFAULT_TOT_CHARGE
for the class.
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 solve it but i didnt implement the _DEFAULT_TOT_CHARGE for the class, since that is a function
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.
Two nitpicks, then it is fine to go. I approved it.
description = ipw.HTML("""Select the advanced settings for the <b>pw.x</b> code.""") | ||
# set here defaul values for the advanced settings | ||
tot_charge_default = 0 | ||
|
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.
Okay, no problem from my side. Thanks!
] = self.advanced_settings.smearing.smearing.value | ||
|
||
# smearing degauss setting | ||
pw_overrides[key]["pw"]["parameters"]["SYSTEM"][ |
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, this is more clear, nice!
Co-authored-by: Jusong Yu <jusong.yeu@gmail.com>
Co-authored-by: Jusong Yu <jusong.yeu@gmail.com>
@unkcpz I accepted your changes, thank you for all the input and help! |
@AndresOrtegaGuerrero thanks a lot for such nice work! Looking forward to reviewing more for new features. |
This PR updates #377 to be compatible with the new re factoring