Feature/adv tot charge #402

AndresOrtegaGuerrero · 2023-05-23T10:32:51Z

This PR updates #377 to be compatible with the new re factoring

codecov · 2023-05-24T13:06:12Z

Codecov Report

Patch coverage: 91.20% and project coverage change: +1.60 🎉

Comparison is base (c16de4e) 50.53% compared to head (9af972f) 52.13%.

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

Flag	Coverage Δ
python-3.10	`52.13% <91.20%> (+1.60%)`	⬆️

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

Impacted Files	Coverage Δ
aiidalab_qe/app/report.py	`28.57% <0.00%> (-0.47%)`	⬇️
aiidalab_qe/app/steps.py	`69.07% <90.78%> (+3.62%)`	⬆️
tests/conftest.py	`97.34% <100.00%> (+0.28%)`	⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

AndresOrtegaGuerrero · 2023-05-24T13:50:58Z

@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 ??

mbercx · 2023-05-24T15:47:27Z

@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 4.3.0 is suddenly failing...

mbercx · 2023-05-24T16:14:09Z

Probably related to explosion/spaCy#12659

mbercx · 2023-05-24T16:51:13Z

Was most likely due to a bug in pydantic~=1.10, now fixed in 1.10.8. I've opened a PR to adapt the dependency:

aiidateam/aiida-quantumespresso#941

I reran some tests that were failing for an aiida-quantumespresso PR, which now pass. But here the pydantic version seems somehow restricted, since the CI is installing version 1.10.2. Not sure why, but let's see what happens after my PR is merged and release a hotfix version 3.4.1.

AndresOrtegaGuerrero · 2023-05-24T18:28:14Z

Was most likely due to a bug in pydantic~=1.10, now fixed in 1.10.8. I've opened a PR to adapt the dependency:

aiidateam/aiida-quantumespresso#941

I reran some tests that were failing for an aiida-quantumespresso PR, which now pass. But here the pydantic version seems somehow restricted, since the CI is installing version 1.10.2. Not sure why, but let's see what happens after my PR is merged and release a hotfix version 3.4.1.

Thank you !

unkcpz

@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

unkcpz · 2023-05-24T23:11:49Z

aiidalab_qe/app/steps.py

@@ -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"]:


Suggested change

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?

Yeah it can be None, if the user do not apply advanced settings override, or the tot_charge override

aiidalab_qe/app/steps.py

unkcpz · 2023-05-24T23:22:35Z

aiidalab_qe/app/static/workflow_summary.jinja

@@ -92,6 +92,10 @@
        <td>{{ nscf_kpoints_distance }} &#8491;<sup>-1</sup></td>
      </tr>
      {% endif %}
+      {% if tot_charge %}


I don't think this logic is needed, if tot_charge not set, it is 0 by default, we can show it as well.

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)

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.

@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 ?

I removed the logic

aiidalab_qe/app/steps.py

unkcpz · 2023-05-30T14:13:41Z

@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 pytest tests/ --force-regen (https://pytest-regressions.readthedocs.io/en/latest/overview.html#force-regen).

…aiidalab-qe into feature/adv_tot_charge

AndresOrtegaGuerrero · 2023-05-30T15:07:09Z

@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 pytest tests/ --force-regen (https://pytest-regressions.readthedocs.io/en/latest/overview.html#force-regen).

@unkcpz I just updated the conftest

AndresOrtegaGuerrero · 2023-05-30T15:13:47Z

@unkcpz , there was a change in the 557a460 I need to another commit

AndresOrtegaGuerrero · 2023-05-30T15:20:22Z

@unkcpz Now it should be fine

unkcpz

@AndresOrtegaGuerrero Thanks again! Still, some minor requests, but the overall shape is very satisfactory.

unkcpz · 2023-05-30T20:48:57Z

aiidalab_qe/app/steps.py

+        self.smearing_settings = SmearingSettings()
+        self.kpoints_settings = KpointSettings()


You use self.advanced_settings.smearing_settings multiple times. It is better to have an interface for the class. See comment above.

unkcpz · 2023-05-30T20:49:50Z

aiidalab_qe/app/steps.py

+    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
+


Suggested change

smeaning_settings = tl.Instance(SmearingSettings())

kpoints_settings = tl.Instance(KpointsSettings())

Add interfaces for the parent class.

If I use this here i get an error, however the interface is establish with AdvancedSetting in ConfigureQeAppWorkChainStep and SubmitQeAppWorkChainStep.

What is the error you got exactly?

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

Okay, no problem from my side. Thanks!

unkcpz · 2023-05-30T20:57:23Z

aiidalab_qe/app/steps.py

-                    "kpoints_distance_override"
-                ]
-                self.kpoints_settings.override_protocol_kpoints.value = True
+                self.advanced_settings.kpoints_settings.kpoints_distance.value = (


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 and kpoints_settings both has "settings", maybe kpoints_settings -> kpoints?

kpoints_distance is under kpoints_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?

unkcpz · 2023-05-30T21:00:13Z

aiidalab_qe/app/static/workflow_summary.jinja

@@ -92,6 +92,10 @@
        <td>{{ nscf_kpoints_distance }} &#8491;<sup>-1</sup></td>
      </tr>
      {% endif %}
+      {% if tot_charge %}


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)

unkcpz

Almost there, three tiny requests.

unkcpz · 2023-05-31T13:54:34Z

aiidalab_qe/app/steps.py

+    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
+


What is the error you got exactly?

unkcpz · 2023-05-31T14:02:59Z

aiidalab_qe/app/steps.py

+                        ] = self.advanced_settings.smearing.smearing.value
+
+                        # smearing degauss setting
+                        pw_overrides[key]["pw"]["parameters"]["SYSTEM"][


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.

@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": {}}}.

I see, this is more clear, nice!

unkcpz · 2023-05-31T14:03:07Z

aiidalab_qe/app/steps.py

@@ -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"]


It will fail if SYSTEM namelist don't have tot_charge, correct?

Yes, I will modify this

unkcpz · 2023-05-31T14:05:11Z

aiidalab_qe/app/steps.py

+        tot_charge = builder.relax.base["pw"]["parameters"]["SYSTEM"]["tot_charge"]
+        if tot_charge:
+            parameters["tot_charge"] = tot_charge
+        else:
+            parameters["tot_charge"] = 0.0
+


Suggested change

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.

@unkcpz I solve it but i didnt implement the _DEFAULT_TOT_CHARGE for the class, since that is a function

unkcpz

Two nitpicks, then it is fine to go. I approved it.

aiidalab_qe/app/report.py

unkcpz · 2023-05-31T15:07:47Z

aiidalab_qe/app/steps.py

+    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
+


Okay, no problem from my side. Thanks!

unkcpz · 2023-05-31T15:09:18Z

aiidalab_qe/app/steps.py

+                        ] = self.advanced_settings.smearing.smearing.value
+
+                        # smearing degauss setting
+                        pw_overrides[key]["pw"]["parameters"]["SYSTEM"][


I see, this is more clear, nice!

tests/conftest.py

Co-authored-by: Jusong Yu <jusong.yeu@gmail.com>

AndresOrtegaGuerrero · 2023-05-31T15:20:02Z

@unkcpz I accepted your changes, thank you for all the input and help!

unkcpz · 2023-05-31T21:52:57Z

@AndresOrtegaGuerrero thanks a lot for such nice work! Looking forward to reviewing more for new features.

AndresOrtegaGuerrero added 4 commits May 23, 2023 08:06

first commit

83974f6

second

d31d7f4

adding report and override modifications

23d69f1

pre-commit fix

edca61a

AndresOrtegaGuerrero requested review from unkcpz and superstar54 May 23, 2023 10:32

pre-commit fix

0d79a70

update test

432ba74

unkcpz mentioned this pull request May 24, 2023

README.md: how to run unit test #403

Merged

unkcpz requested changes May 24, 2023

View reviewed changes

AndresOrtegaGuerrero added 3 commits May 30, 2023 09:03

changes suggested by Jusong

fd96f5b

changes suggested by Jusong

67a5c7e

merging kpoins and smearing in advancedsettings widget

dcb44db

AndresOrtegaGuerrero requested a review from unkcpz May 30, 2023 11:37

Merge branch 'main' into feature/adv_tot_charge

557a460

AndresOrtegaGuerrero added 2 commits May 30, 2023 14:59

updating conftest

a741d97

Merge branch 'feature/adv_tot_charge' of https://github.com/aiidalab/…

475f7c3

…aiidalab-qe into feature/adv_tot_charge

updating Notebook to include advanced settings

5ac1fa1

unkcpz requested changes May 30, 2023

View reviewed changes

class TotalCharge, update of KpointsSettings and SmearingSetting to r…

cae7b0b

…educe ambiguity (Jusong)

AndresOrtegaGuerrero added 2 commits May 31, 2023 08:19

class TotalCharge, update of KpointsSettings and SmearingSetting to r…

e3f7de9

…educe ambiguity (Jusong)

removing tot_charge logic, setting to parameters to 0.0 as default

759f751

AndresOrtegaGuerrero requested a review from unkcpz May 31, 2023 09:54

unkcpz requested changes May 31, 2023

View reviewed changes

updating _extract_report_parameters

a8740de

AndresOrtegaGuerrero requested a review from unkcpz May 31, 2023 14:42

unkcpz approved these changes May 31, 2023

View reviewed changes

AndresOrtegaGuerrero and others added 2 commits May 31, 2023 17:15

Update aiidalab_qe/app/report.py

793af07

Co-authored-by: Jusong Yu <jusong.yeu@gmail.com>

Update tests/conftest.py

9af972f

Co-authored-by: Jusong Yu <jusong.yeu@gmail.com>

unkcpz merged commit 827bb83 into main May 31, 2023
7 of 11 checks passed

unkcpz deleted the feature/adv_tot_charge branch May 31, 2023 21:56

	if builder.relax.base["pw"]["parameters"]["SYSTEM"]["tot_charge"]:
	tot_charge = builder.relax.base["pw"]["parameters"]["SYSTEM"]["tot_charge"]
	if total_charge:

		self.smearing_settings = SmearingSettings()
		self.kpoints_settings = KpointSettings()


	smeaning_settings = tl.Instance(SmearingSettings())
	kpoints_settings = tl.Instance(KpointsSettings())

Feature/adv tot charge #402

Feature/adv tot charge #402

Conversation

AndresOrtegaGuerrero commented May 23, 2023

codecov bot commented May 24, 2023 • edited Loading

Codecov Report

AndresOrtegaGuerrero commented May 24, 2023

mbercx commented May 24, 2023

mbercx commented May 24, 2023

mbercx commented May 24, 2023

AndresOrtegaGuerrero commented May 24, 2023

unkcpz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndresOrtegaGuerrero May 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndresOrtegaGuerrero May 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unkcpz commented May 30, 2023 • edited Loading

AndresOrtegaGuerrero commented May 30, 2023

AndresOrtegaGuerrero commented May 30, 2023

AndresOrtegaGuerrero commented May 30, 2023

unkcpz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unkcpz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unkcpz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndresOrtegaGuerrero commented May 31, 2023

unkcpz commented May 31, 2023

codecov bot commented May 24, 2023 •

edited

Loading

AndresOrtegaGuerrero May 26, 2023 •

edited

Loading

AndresOrtegaGuerrero May 26, 2023 •

edited

Loading

unkcpz commented May 30, 2023 •

edited

Loading