Feature: Starting Magnetization #409

AndresOrtegaGuerrero · 2023-06-01T11:09:58Z

This PR implements a class MagnetizationSettings for Advanced Settings in the QE App, to be able to set initial_magnetic_moments in the calculations. Address #238
The user can select the magnetization per kind defined in the input_structure

The will be another PR for an AddingTags Editor, so the user can create new tags for the atoms in the structure.

mbercx · 2023-06-01T11:52:04Z

Hi @AndresOrtegaGuerrero and @unkcpz! I will be very busy the next few weeks on other projects, so unless there is something specific needed from my end (i.e. on the aiida-quantumespresso plugin), I will not have time to review PRs on the QEapp.

In general, I've been less and less involved on the development there. I am simply spread too thin otherwise and then no work gets done anywhere. Apologies.

…oments

AndresOrtegaGuerrero · 2023-06-01T12:25:17Z

Hi @AndresOrtegaGuerrero and @unkcpz! I will be very busy the next few weeks on other projects, so unless there is something specific needed from my end (i.e. on the aiida-quantumespresso plugin), I will not have time to review PRs on the QEapp.

In general, I've been less and less involved on the development there. I am simply spread too thin otherwise and then no work gets done anywhere. Apologies.

No worries, I understand, thank you!

codecov · 2023-06-01T12:31:02Z

Codecov Report

Patch coverage: 86.13% and project coverage change: +1.66 🎉

Comparison is base (9df98ea) 52.63% compared to head (60723ab) 54.29%.

❗ Current head 60723ab differs from pull request most recent head 08558b4. Consider uploading reports for the commit 08558b4 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #409      +/-   ##
==========================================
+ Coverage   52.63%   54.29%   +1.66%     
==========================================
  Files          17       17              
  Lines        1879     1978      +99     
==========================================
+ Hits          989     1074      +85     
- Misses        890      904      +14

Flag	Coverage Δ
python-3.10	`54.29% <86.13%> (+1.66%)`	⬆️
python-3.8	`54.35% <86.13%> (+1.66%)`	⬆️
python-3.9	`54.35% <86.13%> (+1.66%)`	⬆️

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

Impacted Files	Coverage Δ
aiidalab_qe/app/report.py	`28.12% <0.00%> (-0.45%)`	⬇️
tests/test_submit_qe_workchain.py	`93.15% <83.33%> (-4.85%)`	⬇️
aiidalab_qe/app/steps.py	`70.88% <86.15%> (+1.96%)`	⬆️
tests/conftest.py	`97.58% <100.00%> (+0.23%)`	⬆️

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

unkcpz · 2023-06-01T12:51:07Z

Giovanni mentioned there will be a discussion for the structure data for this on 9th June, I asked him to invite @AndresOrtegaGuerrero. It might be better to wait for that one.

unkcpz · 2023-06-19T16:22:48Z

As discussed in the 9th June meeting, summarized at https://hackmd.io/xLA_d3AaSaqhAGVZIhdP7A?view. In QeApp, we don't plan to go that way in the moment but I am agree with @AndresOrtegaGuerrero to do the change from directly widget side.
The pros are we are independent from the core packges and StructureData, and thus can flexiblelly start proceed without blocking, the cons are not such magnetization settings will store in the database therefore impossible for query. But the current StructureData is not easy to query as well, so nothing to lose.
Once we have the widget, it should be not hard to change the backend to support the new extended StructureData in the future.

I'll give it a review.

unkcpz · 2023-06-28T08:03:19Z

A quick comment, if I didn't select structure, there is a dangling "override" checkbox there which looks a bit weird. I'd suggest having a text under the "Total charge" showing it is the placeholder for setting the initial magnetic moments.

unkcpz

@AndresOrtegaGuerrero thanks for adding this! I only have some minor requests, the overall implementation looks fine, cheers!

aiidalab_qe/app/static/workflow_summary.jinja

aiidalab_qe/app/steps.py

unkcpz · 2023-06-28T08:15:22Z

aiidalab_qe/app/steps.py

+class MagnetizationSettings(ipw.VBox):
+    input_structure = traitlets.Instance(StructureData, allow_none=True)


Can you add a docstring to this widget to describe what input is expected for input_structure_labels, and give an example of it? It is not very clear from the first glimpse.

unkcpz · 2023-06-28T08:15:46Z

aiidalab_qe/app/steps.py

+        )
+        self.override.observe(self._disable_kings_widgets, "value")
+
+    def _disable_kings_widgets(self, _=None):


Suggested change

def _disable_kings_widgets(self, _=None):

def _disable_kinds_widgets(self, _=None):

😃 who is the king?

unkcpz · 2023-06-28T08:22:34Z

aiidalab_qe/app/steps.py

+
+    def update_kinds_widgets(self, change):
+        self.input_structure_labels = self.input_structure.get_kind_names()
+        self.kinds = self.create_kinds_widgets()


If this is the first time the self.kinds is initialized and used, I suggest in the __init__ function, call the update and also set a placeholder text something like "Your input structure does not contain magnetic elements or the magnetic calculation is not turned on."

unkcpz · 2023-06-28T08:24:49Z

aiidalab_qe/app/steps.py

@@ -975,6 +1061,14 @@ def _get_qe_workchain_parameters(self) -> QeWorkChainParameters:
                    pw_overrides[key]["pw"]["parameters"]["SYSTEM"][
                        "tot_charge"
                    ] = self.advanced_settings.tot_charge.charge.value
+                if (
+                    self.advanced_settings.magnetization.override.value
+                    and self.workchain_settings.spin_type.value == "collinear"


I am a bit confused, is "non-collinear" also supported? I didn't see the widget to set the spin_type to "collinear".

To define if you are using spin is

self.spin_type = ipw.ToggleButtons( options=[("Off", "none"), ("On", "collinear")], value=DEFAULT_PARAMETERS["spin_type"], style={"description_width": "initial"}, )

I see, fair. 👍

unkcpz · 2023-06-28T08:27:49Z

aiidalab_qe/app/steps.py

@@ -1034,6 +1128,7 @@ def _get_qe_workchain_parameters(self) -> QeWorkChainParameters:
            spin_type=spin_type,
            electronic_type=electronic_type,
            overrides=overrides,
+            initial_magnetic_moments=starting_magnetization,


Do we really need different names? Can all be called "initial_magnetic_moments" or "starting_magnetiziation"?

unkcpz · 2023-06-28T08:27:57Z

aiidalab_qe/app/steps.py

@@ -1034,6 +1128,7 @@ def _get_qe_workchain_parameters(self) -> QeWorkChainParameters:
            spin_type=spin_type,
            electronic_type=electronic_type,
            overrides=overrides,
+            initial_magnetic_moments=starting_magnetization,


Do we really need different names? Can all be called "initial_magnetic_moments" or "starting_magnetiziation"?

i will update this

unkcpz · 2023-06-28T08:34:07Z

tests/test_submit_qe_workchain.py

+    assert (
+        report_html.count("None") <= 1
+        or builder_parameters("initial_magnetic_moments") is not None
+    )


thanks for the test!
I think the test can be more specific, like:

from bs4 import BeautifulSoup if builder_parameters("initial_magnetic_moments") is None: parsed = BeautifulSoup(report_html) assert parsed.??.text == "None"

unkcpz · 2023-06-28T08:34:21Z

aiidalab_qe/app/steps.py

+
+    def __init__(self, **kwargs):
+        self.input_structure = StructureData()
+        self.kinds = self.create_kinds_widgets()


You initialize the self.kinds but didn't use it in __init__ function. Maybe it makes sense to create and use it when it is used. See the comment below in function update_kinds_widgets.

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

AndresOrtegaGuerrero · 2023-06-30T14:58:25Z

@unkcpz Thank you for your suggestions and help. I just added another test for advanced settings and adapted your suggestions. Let me know what you think.

unkcpz

Thanks @AndresOrtegaGuerrero, three minor requests. It is all fine then.

unkcpz · 2023-07-01T22:01:16Z

aiidalab_qe/app/steps.py

@@ -318,6 +325,107 @@ def reset(self):
            self.override.value = False


+class MagnetizationSettings(ipw.VBox):
+    """Widget to set the initial_magnetic_moments from each Kind in StructureData


Suggested change

"""Widget to set the initial_magnetic_moments from each Kind in StructureData

"""Widget to set initial magnetic moments for each `Kind` (usually the element name, but you can use `Fe1`, `Fe2` to distinguish iron atoms of different sites) of `StructureData`

Am I rephrasing it correctly? Please feel free to polish what I wrote.

Thank you i will expand this 👍

unkcpz · 2023-07-01T22:07:02Z

aiidalab_qe/app/steps.py

+            widgets_list = []
+            for label in self.input_structure_labels:
+                hbox = ipw.HBox()
+                float_widget = ipw.BoundedFloatText(


No a big issue since it as a local variable, but I prefer bounded_float or simply w. float_widget seems a bit like a widget that can float on the webpage. Feel free to ignore this comment.

And why not do like this?

hbox = ipw.HBox( children=[ ipw.BoundedFloatText(... ] )

unkcpz · 2023-07-01T22:09:22Z

aiidalab_qe/app/steps.py

+                )
+                hbox.children = [float_widget]
+                widgets_list.append(hbox)
+            kinds_widget = ipw.VBox([ipw.HTML("Define magnetization")] + widgets_list)


Here you didn't use HTML tags for formatting, is ipw.HTML really needed?

I will modify this

unkcpz · 2023-07-01T22:24:12Z

aiidalab_qe/app/steps.py

+            for i in range(1, len(self.kinds.children)):
+                self.kinds.children[i].children[0].value = 0.0


I understand why it has to set the value like this, from range 1 to skip the HTML widget of index 0, and second children for the HBox. But it is a bit complex and error-prone when looking at this loop independently. Not sure there is a better way, I think having a Kind widget might be too much.
Just think about it, if there is not better way to make it clear, leave it as this and add a comment for why start loop from index 1 and what the second children refer to.

I will update in such a way that is a VBox instead and to access the valees is self.kinds.children[i].value

unkcpz · 2023-07-01T22:26:21Z

aiidalab_qe/app/steps.py

@@ -975,6 +1061,14 @@ def _get_qe_workchain_parameters(self) -> QeWorkChainParameters:
                    pw_overrides[key]["pw"]["parameters"]["SYSTEM"][
                        "tot_charge"
                    ] = self.advanced_settings.tot_charge.charge.value
+                if (
+                    self.advanced_settings.magnetization.override.value
+                    and self.workchain_settings.spin_type.value == "collinear"


I see, fair. 👍

tests/test_submit_qe_workchain.py

unkcpz · 2023-07-01T22:28:10Z

tests/test_submit_qe_workchain.py

+        assert parsed.find("initial_magnetic_moments").text == "None"
+
+
+def test_create_builder_advanced_settings(


Thanks a lot for this test, very useful!

Yes i think here we can add the test for new advanced settings :)

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

AndresOrtegaGuerrero · 2023-07-02T17:13:23Z

@unkcpz Let me know i adapted the code :)

unkcpz

Hi @AndresOrtegaGuerrero, It is all fine. I have one question about the PYTEST_CURRENT_TEST, but I approve it anyway since the logic is all great for me.

unkcpz · 2023-07-03T13:05:25Z

aiidalab_qe/app/steps.py

+        self.display_kinds()
+
+    def display_kinds(self):
+        if "PYTEST_CURRENT_TEST" not in os.environ and self.kinds:


Interesting, I didn't know this PYTEST_CURRENT_TEST. Can you double check this is really needed here? I think even is a test it should be found to clear_output and display. Am I missing something?

@unkcpz If i dont use this , it will return the widget during the pytest, so to avoid this behaviour i use the display and clear only if i am not in a pytest , but i do the update before.

Sorry, I didn't understand, what will happen if the widget is returned during pytest, the test will fail? with what exception?

@unkcpz The test wont fail , but during the test in the terminal it will print the widget as text

Did you use pytest -s? Other wise it should not print things. If this is not needed, I'd not add it.
Sorry for the back-and-forth on this PR.

@unkcpz No worries, is good to double check. I did the test with pytest -s without this line and the issue occurs, the test passed but it returns the object in this case a VBox. The line is needed. I push an update in the documentation of the test so is more clear.

I did the test with pytest -s

Yep, -s is to print things to the stdout. I think it is fair to suppress it with your way, otherwise run with -s locally would be a mess.

All good to me now. Do you mind merge this one with writing a description of the commit message? Just click the "squash and merge" below and input the message in the dialog.

*This PR implements an Editor for the QE App, to enable the user to define extra kinds within a structure.This editor is intended to facilitated spin polarisation calculations with #409 Co-authored-by: Xing Wang <xingwang1991@gmail.com>

starting magnetization feature

afc57cb

AndresOrtegaGuerrero requested review from unkcpz and mbercx June 1, 2023 11:10

AndresOrtegaGuerrero mentioned this pull request Jun 1, 2023

Feature/ Tags editor to define Kinds in StructureData #410

Merged

fixing test_submit_qe_workchain to allow None from initial_magnetic_m…

595983d

…oments

AndresOrtegaGuerrero requested review from superstar54 and removed request for mbercx June 1, 2023 12:25

Merge branch 'main' into feature/magnetization

34cbabb

unkcpz requested changes Jun 28, 2023

View reviewed changes

AndresOrtegaGuerrero and others added 2 commits June 28, 2023 11:55

Update aiidalab_qe/app/steps.py

686e77d

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

adapting changes suggested by Jusong

4b1d096

AndresOrtegaGuerrero requested a review from unkcpz June 30, 2023 16:39

unkcpz requested changes Jul 1, 2023

View reviewed changes

AndresOrtegaGuerrero and others added 2 commits July 2, 2023 16:09

Update tests/test_submit_qe_workchain.py

620a81b

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

Adjusting suggestion from Jusong

60723ab

AndresOrtegaGuerrero requested a review from unkcpz July 2, 2023 17:13

unkcpz approved these changes Jul 3, 2023

View reviewed changes

updating documentation

08558b4

AndresOrtegaGuerrero merged commit 354c81a into main Jul 4, 2023
12 checks passed

AndresOrtegaGuerrero deleted the feature/magnetization branch July 4, 2023 14:35

AndresOrtegaGuerrero mentioned this pull request Sep 26, 2023

Add a widget to assign the spin guesses. #238

Closed

		class MagnetizationSettings(ipw.VBox):
		input_structure = traitlets.Instance(StructureData, allow_none=True)

	def _disable_kings_widgets(self, _=None):
	def _disable_kinds_widgets(self, _=None):

	"""Widget to set the initial_magnetic_moments from each Kind in StructureData
	"""Widget to set initial magnetic moments for each `Kind` (usually the element name, but you can use `Fe1`, `Fe2` to distinguish iron atoms of different sites) of `StructureData`

		for i in range(1, len(self.kinds.children)):
		self.kinds.children[i].children[0].value = 0.0

		assert parsed.find("initial_magnetic_moments").text == "None"


		def test_create_builder_advanced_settings(

Feature: Starting Magnetization #409

Feature: Starting Magnetization #409

Conversation

AndresOrtegaGuerrero commented Jun 1, 2023 • edited Loading

mbercx commented Jun 1, 2023

AndresOrtegaGuerrero commented Jun 1, 2023

codecov bot commented Jun 1, 2023 • edited Loading

Codecov Report

unkcpz commented Jun 1, 2023

unkcpz commented Jun 19, 2023

unkcpz commented Jun 28, 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

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

AndresOrtegaGuerrero commented Jun 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndresOrtegaGuerrero commented Jul 2, 2023

unkcpz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndresOrtegaGuerrero Jul 3, 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

AndresOrtegaGuerrero Jul 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndresOrtegaGuerrero commented Jun 1, 2023 •

edited

Loading

codecov bot commented Jun 1, 2023 •

edited

Loading

AndresOrtegaGuerrero Jul 3, 2023 •

edited

Loading

AndresOrtegaGuerrero Jul 4, 2023 •

edited

Loading