add `electronic_structure` plugin #522

superstar54 · 2023-10-21T06:19:36Z

This PR supports

showing the result from multiple plugins.
only shows the result panel when it is available.

One can use the workchain_labels to specify which plugins (outputs) are needed for this result panel. For example, the electronic_structure result panel needs pdos and bands.

class Result(ResultPanel):
    title = "Electronic Structure"
    workchain_labels = ["bands", "pdos"]

codecov · 2023-10-21T06:46:06Z

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (b731978) 78.92% compared to head (5b486fd) 79.07%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #522      +/-   ##
==========================================
+ Coverage   78.92%   79.07%   +0.14%     
==========================================
  Files          45       47       +2     
  Lines        3213     3211       -2     
==========================================
+ Hits         2536     2539       +3     
+ Misses        677      672       -5

Flag	Coverage Δ
python-3.10	`79.07% <94.64%> (+0.14%)`	⬆️
python-3.8	`79.11% <94.64%> (+0.14%)`	⬆️
python-3.9	`79.11% <94.64%> (+0.14%)`	⬆️

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

Files	Coverage Δ
src/aiidalab_qe/common/panel.py	`86.95% <100.00%> (+0.28%)`	⬆️
src/aiidalab_qe/plugins/bands/result.py	`94.73% <100.00%> (+0.29%)`	⬆️
...idalab_qe/plugins/electronic_structure/__init__.py	`100.00% <100.00%> (ø)`
src/aiidalab_qe/plugins/pdos/result.py	`92.95% <100.00%> (+0.10%)`	⬆️
tests/test_plugins_bands.py	`100.00% <ø> (ø)`
tests/test_plugins_electronic_structure.py	`100.00% <100.00%> (ø)`
tests/test_plugins_pdos.py	`100.00% <ø> (ø)`
tests/test_result.py	`100.00% <ø> (ø)`
...aiidalab_qe/plugins/electronic_structure/result.py	`79.26% <94.44%> (ø)`
src/aiidalab_qe/app/result/workchain_viewer.py	`57.40% <88.88%> (-2.06%)`	⬇️

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

unkcpz

Thanks @superstar54, I have two things ask to change, please have a look.

unkcpz · 2023-10-23T11:13:40Z

tests/test_result.py

-    wcv = WorkChainViewer(wkchain.node)
-    assert (
-        "DOS grouped by:"
-        == wcv.result_tabs.children[2].children[0].children[0].children[0].value


I have no idea of all these children are, a detailed comment will be very useful.
Since now things are implemented as "Panel", is some of these children are panel? If so, I think Panel can have some way to associate children with name to make it easy to access.

The test here is too detailed, and not necessary, so I removed it. A new test that checks the length of the panel children is added.

Nice, that is better!

unkcpz · 2023-10-23T11:16:25Z

src/aiidalab_qe/plugins/electronic_structure/result.py

+            children=[
+                ipw.HBox(
+                    children=[
+                        ipw.Label(
+                            "DOS grouped by:",
+                            layout=ipw.Layout(
+                                justify_content="flex-start", width="120px"
+                            ),
+                        ),
+                        group_dos_by,
+                    ]
+                ),
+            ],
+            layout={"margin": "0 0 30px 30px"},
+        )


Please avoid a deep nest of widgets, if there are many, define it outside with a comment on what the box is for and where it is displayed.

I refactored the code. Now, it's become more simple and clear.

unkcpz · 2023-10-23T11:18:56Z

BTW, there is no need to always merge the branch, if I find time to review I'll merge the main before I review. Usually, this happens before the final merge if there is no conflict.

superstar54 · 2023-10-27T13:25:45Z

@unkcpz I refactored the code and added detailed comments. It's ready for review.

Please focus on the plugin part, which is the primary purpose of this PR.

The part of plotting the electronic structure is simply copied and pasted from the previous code, with few updates. If the code needs polished, it is better to open another PR.

unkcpz

Hi @superstar54, thanks a lot for refactoring, very beautiful! Just some minor questions.

unkcpz · 2023-10-27T15:16:06Z

src/aiidalab_qe/app/result/workchain_viewer.py

-            bands=bands_data,
-            dos=dos_data,
-        )
+                    if False not in results_ready:


Suggested change

if False not in results_ready:

if all(results_ready):

unkcpz · 2023-10-27T15:17:42Z

src/aiidalab_qe/app/result/workchain_viewer.py

+                        # add this plugin result panel
+                        self.result_tabs.children += (result,)
+                        self.result_tabs.set_title(
+                            len(self.result_tabs.children) - 1, result.title


I guess len(self.result_tabs.children) - 1 is the idx of the tab. Please have a variable for it with comment.

unkcpz · 2023-10-27T15:18:55Z

src/aiidalab_qe/plugins/bands/result.py

@@ -29,14 +29,15 @@ class Result(ResultPanel):
    """Result panel for the bands calculation."""

    title = "Bands"
+    workchain_labels = ["bands"]


What is this labels for? If there are more than one where will those shown?

One can use the workchain_labels to specify which plugins (outputs) are needed for this result panel. For example, the electronic_structure result panel needs pdos and bands.

workchain_labels = ["bands", "pdos"]

To make this clear, I added a comment in the ResultPanel.

unkcpz · 2023-10-27T15:21:27Z

tests/test_result.py

-    wcv = WorkChainViewer(wkchain.node)
-    assert (
-        "DOS grouped by:"
-        == wcv.result_tabs.children[2].children[0].children[0].children[0].value


Nice, that is better!

unkcpz · 2023-10-27T15:24:17Z

tests/test_plugins_electronic_structure.py

+    tab = [
+        tab
+        for tab in wcv.result_tabs.children
+        if getattr(tab, "identifier", "") == "electronic_structure"


I assume identifier is a mendatory attribute of panel, so get this be:

Suggested change

if getattr(tab, "identifier", "") == "electronic_structure"

if tab.identifier == "electronic_structure"

Please take a look at the comment above the code.

# the built-in summary and structure tabs is not a plugin panel, # thus don't have identifiers ```

superstar54 · 2023-10-27T19:05:07Z

@unkcpz I updated the code based on your suggestions. It's ready for another review.

unkcpz

Thanks @superstar54 ! All good for me. Ready to 🚢

…` plugin (#522) This PR supports: - showing the result from multiple plugins. - only shows the result panel when it is available. - use `electronic_structure` plugin as an example, in which the result panel needs both `pdos` and `bands` plugin.

superstar54 added 2 commits October 21, 2023 06:18

add electronic_structure plugin

16f0219

fix test

5f29b3e

superstar54 requested a review from unkcpz October 21, 2023 06:48

superstar54 added the enhancement New feature or request label Oct 21, 2023

superstar54 added 3 commits October 21, 2023 16:18

Merge branch 'main' into feature/result_multiple_plugins

2a627f7

Merge branch 'main' into feature/result_multiple_plugins

09c9c30

Merge branch 'main' into feature/result_multiple_plugins

0d24f5e

unkcpz requested changes Oct 23, 2023

View reviewed changes

superstar54 added 4 commits October 27, 2023 13:43

Merge branch 'main' into feature/result_multiple_plugins

22e0f20

update _update_view

59353d2

refactor the result panel

0a27bee

add comment

f52ec99

unkcpz requested changes Oct 27, 2023

View reviewed changes

improve code and add comment

5b486fd

superstar54 requested a review from unkcpz October 27, 2023 20:30

unkcpz approved these changes Oct 29, 2023

View reviewed changes

superstar54 merged commit 8a550c8 into main Oct 29, 2023
15 of 17 checks passed

superstar54 deleted the feature/result_multiple_plugins branch October 29, 2023 06:05

unkcpz restored the feature/result_multiple_plugins branch November 2, 2023 15:56

This was referenced Nov 3, 2023

Feature/result multiple plugins #545

Merged

Some layout issues that deteriorate UX #528

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add `electronic_structure` plugin #522

add `electronic_structure` plugin #522

superstar54 commented Oct 21, 2023 •

edited

Loading

codecov bot commented Oct 21, 2023 •

edited

Loading

unkcpz left a comment

unkcpz Oct 23, 2023

superstar54 Oct 27, 2023

unkcpz Oct 27, 2023

unkcpz Oct 23, 2023

superstar54 Oct 27, 2023

unkcpz commented Oct 23, 2023

superstar54 commented Oct 27, 2023

unkcpz left a comment

unkcpz Oct 27, 2023

unkcpz Oct 27, 2023

unkcpz Oct 27, 2023

superstar54 Oct 27, 2023

unkcpz Oct 27, 2023

unkcpz Oct 27, 2023

superstar54 Oct 27, 2023

superstar54 commented Oct 27, 2023

unkcpz left a comment

	if getattr(tab, "identifier", "") == "electronic_structure"
	if tab.identifier == "electronic_structure"

add electronic_structure plugin #522

add electronic_structure plugin #522

Conversation

superstar54 commented Oct 21, 2023 • edited Loading

codecov bot commented Oct 21, 2023 • edited Loading

Codecov Report

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

unkcpz commented Oct 23, 2023

superstar54 commented Oct 27, 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

superstar54 commented Oct 27, 2023

unkcpz left a comment

Choose a reason for hiding this comment

add `electronic_structure` plugin #522

add `electronic_structure` plugin #522

superstar54 commented Oct 21, 2023 •

edited

Loading

codecov bot commented Oct 21, 2023 •

edited

Loading