Fix bugs in pseudopotentials panel by edan-bainglass · Pull Request #1504 · aiidalab/aiidalab-qe

edan-bainglass · 2026-05-20T17:13:21Z

The panel was incorrectly set when loaded from a process (or duplicated).

Note that this doesn't work on its own due to a larger issue with the blockers system. This is fixed in another PR.

codecov · 2026-05-20T17:19:51Z

Codecov Report

❌ Patch coverage is 81.94444% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.35%. Comparing base (dd9c160) to head (5d0807b).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...rc/aiidalab_qe/app/configuration/advanced/model.py	28.57%	10 Missing ⚠️
...lab_qe/app/configuration/advanced/pseudos/model.py	91.42%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1504      +/-   ##
==========================================
- Coverage   75.42%   75.35%   -0.07%     
==========================================
  Files         115      115              
  Lines        7365     7378      +13     
==========================================
+ Hits         5555     5560       +5     
- Misses       1810     1818       +8

Flag	Coverage Δ
python-3.12	`75.35% <81.94%> (-0.07%)`	⬇️
python-3.9	`75.38% <81.94%> (-0.07%)`	⬇️

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

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

edan-bainglass · 2026-05-20T18:52:29Z

@AndresOrtegaGuerrero I did not add any tests for this. I'll open an issue to add tests, but I don't have time at the moment.

edan-bainglass · 2026-05-22T12:14:09Z

-    def update_functionals(self):
-        if self.locked or not (self.functional and self.family):
-            return
-        self.functionals = (
-            [self.functional] * len(self.input_structure.kinds)
-            if self.has_structure
-            else []
-        )


There is no need for functionals. We have all the information we need for what it was used for. Discarding it to reduce complexity.

edan-bainglass · 2026-05-22T12:37:04Z

+    def get_cutoffs_by_index(self, index: int) -> list[float]:
+        return (
+            [self.cutoffs[0][index], self.cutoffs[1][index]]
+            if len(self.cutoffs[0]) > index
+            else [0.0, 0.0]
+        )


Cleaner to have this helper here than in the settings panel.

edan-bainglass · 2026-05-22T12:38:49Z

+    def update_functional(self):
+        pseudos: list[UpfData] = []
+        for uuid in self.dictionary.values():
+            try:
+                assert uuid is not None
+                pseudo = orm.load_node(uuid)
+                pseudos.append(pseudo)
+            except (AssertionError, exceptions.NotExistent):
+                continue
+        functional_set = {pp.base.extras.get("functional", None) for pp in pseudos}
+        functional = functional_set.pop() if len(functional_set) == 1 else None
+        self._defaults["functional"] = functional
+        self.functional = self._get_default("functional")


This is called on change of pseudos in the dictionary. It collects each pseudo's functional (from extras), and sets the functional trait to the common functional if exists, otherwise None - functional is disabled in the UI.

edan-bainglass · 2026-05-22T12:40:46Z

@@ -288,65 +307,23 @@ def on_default_pseudo(
                    pseudo = orm.load_node(self._model.dictionary.get(kind_name))
                except Exception:
                    pseudo = None
+                model.uploaded = model.pseudo is pseudo
                model.pseudo = pseudo
-                model.cutoffs = (
-                    [
-                        self._model.cutoffs[0][index],  # type: ignore
-                        self._model.cutoffs[1][index],  # type: ignore
-                    ]
-                    if len(self._model.cutoffs[0]) > index  # type: ignore
-                    else [0.0, 0.0]
-                )
+                model.cutoffs = self._model.get_cutoffs_by_index(index)
                model.update_pseudo_info()
-                model.uploaded = False

            self._model.observe(
-                on_default_pseudo,
+                synchronize_pseudo,
                "dictionary",
            )

-            def on_pseudo_upload(
-                change,
-                model: PseudoPotentialUploaderModel = uploader_model,
-                kind_name=kind.name,
-                index=index,
-            ):
-                if not (change["new"] and model.pseudo):
-                    return
-
-                self._model.family = None
-                self._model.library = None
-                self._model.functional = None
-
-                self._model.dictionary = {
-                    **self._model.dictionary,
-                    kind_name: model.pseudo.uuid,
-                }
-
-                functional = model.pseudo.base.extras.get("functional", None)
-                functionals = [*self._model.functionals]
-                functionals[index] = functional
-                # The following double-setting is done to force the blockers check,
-                # which now also bail early if the functionals are empty.
-                self._model.functionals = []
-                self._model.functionals = functionals
-
-                cutoffs: list = deepcopy(self._model.cutoffs)  # type: ignore
-                cutoffs[0][index] = model.cutoffs[0]  # type: ignore
-                cutoffs[1][index] = model.cutoffs[1]  # type: ignore
-                self._model.cutoffs = cutoffs
-
-            uploader_model.observe(
-                on_pseudo_upload,
-                "uploaded",
-            )
-
+            uploader = PseudoPotentialUploader(model=uploader_model)
            uploader.render()

-            on_default_pseudo()
-
            self._links.extend(uploader.links)

            children.append(uploader)

+            synchronize_pseudo()


This is mostly renaming and reshuffling for clarity, plus adjusting the logic w.r.t dropping functionals.

edan-bainglass · 2026-05-22T14:44:34Z

@@ -218,22 +218,29 @@ def set_model_state(self, state: dict):
                pseudos.library = library
                pseudos.family = pseudo_family_string
            else:
+                # Otherwise, we assume a custom pseudo upload (warning enabled).
+                # Everything is set to None except the functional, which we extract from the pseudos.
+                pseudos.show_upload_warning = True
+                functionals = []
                try:
-                    pp_uuid = next(iter(PW["pseudos"].values()))
-                    pseudo_info = get_pseudo_info(pp_uuid)
-                    pseudos.functional = pseudo_info["functional"]
-                except Exception:
+                    for pp_uuid in PW["pseudos"].values():
+                        pseudo_info = get_pseudo_info(pp_uuid)
+                        functionals.append(pseudo_info["functional"])
+                    if len(set(functionals)) == 1:
+                        pseudos.functional = functionals[0]
+                    else:
+                        pseudos.functional = None
+                except Exception as err:
+                    print(f"Error loading pseudos: {err}")
                    pseudos.functional = None
                pseudos.library = None
                pseudos.family = None
-                pseudos.show_upload_warning = True
-
-            pseudos.functionals = [pseudos.functional] * num_kinds

-            if pseudos_dictionary := PW.get("pseudos"):
-                pseudos.dictionary = pseudos_dictionary
-                pseudos.ecutwfc = SYSTEM.get("ecutwfc", 0.0)
-                pseudos.ecutrho = SYSTEM.get("ecutrho", 0.0)
+        if pseudos_dictionary := PW.get("pseudos"):
+            # We set these outside of the context manager to avoid a reset by other trait notifications.
+            pseudos.dictionary = pseudos_dictionary
+            pseudos.ecutwfc = SYSTEM.get("ecutwfc", 0.0)
+            pseudos.ecutrho = SYSTEM.get("ecutrho", 0.0)


This part I suspect can be simplified, but immediate efforts failed. Keeping as is for now.

The panel was incorrectly set when loaded from a process (or duplicated).

edan-bainglass · 2026-05-30T08:08:29Z

@AndresOrtegaGuerrero did you have any other comments? I'd like to get this in before releasing a new QE app version 🙏 If there are any issues that I missed, we can address them with a patch release next month (including further tests of course).

AndresOrtegaGuerrero · 2026-05-30T08:55:56Z

+        pseudos: list[UpfData] = []
+        for uuid in self.dictionary.values():
+            try:
+                assert uuid is not None


why not if uuid is None: ? , I guess is to put raise an Error ?

uuid is rarely None. In practice, it shouldn't ever be None. Hence, instead of checking for a condition that is rarely there (if ever), I rather try what SHOULD work, catching the rare cases where something is out of sync. Ideally, I would implement a better logging system for debugging and add debug messages in these rare events, to let us know what happened. Actually, though it would be annoying for the user, perhaps it is good to still print out something. I'll update this block, separating the two errors with dedicated print messages.

Looking at some older code (see the _check_blockers method in the same file, I indeed split the two error paths, with the assert error leading to a blocker w.r.t non-existent pseudos in the selected sssp library for the selected element. I recall this being an issue for Ce at some point. I should do something similar for the new code.

Yes, there are missing Psedos for SOC for lanthanides... yes

AndresOrtegaGuerrero

LGMT! I left a comment but it was more like a question

edan-bainglass force-pushed the fix-pseudos branch from 6580499 to 0f03354 Compare May 20, 2026 18:49

edan-bainglass requested a review from AndresOrtegaGuerrero May 20, 2026 18:51

edan-bainglass mentioned this pull request May 20, 2026

Pseudos settings panel could use additional testing #1505

Open

edan-bainglass force-pushed the fix-pseudos branch 2 times, most recently from 1688c59 to ceccbee Compare May 22, 2026 11:34

edan-bainglass commented May 22, 2026

View reviewed changes

edan-bainglass mentioned this pull request May 22, 2026

Fix blockers system #1513

Merged

edan-bainglass requested a review from giovannipizzi May 22, 2026 14:48

AndresOrtegaGuerrero reviewed May 22, 2026

View reviewed changes

Comment thread src/aiidalab_qe/app/configuration/advanced/pseudos/model.py

edan-bainglass requested a review from AndresOrtegaGuerrero May 30, 2026 07:38

edan-bainglass added 9 commits May 30, 2026 07:45

Fix bugs in pseudopotentials panel

e5536a5

The panel was incorrectly set when loaded from a process (or duplicated).

Use bounded widgets for ecuts

b87b530

Update blockers on ecut changes

bffbbb0

Block on null ecuts

5f0eaa4

Fix pseudos panel and model

6939c1c

Update tests

b52b77b

Drop remaining references to functionals

644ba47

Explicitly initialize pseudos traits to None

3ee1754

Simplify populating pseudos from previous state

ec4bbe2

edan-bainglass force-pushed the fix-pseudos branch from bb0d104 to ec4bbe2 Compare May 30, 2026 07:45

AndresOrtegaGuerrero reviewed May 30, 2026

View reviewed changes

AndresOrtegaGuerrero self-requested a review May 30, 2026 08:56

AndresOrtegaGuerrero approved these changes May 30, 2026

View reviewed changes

Handle rare exceptions better when handling pseudos

5d0807b

edan-bainglass force-pushed the fix-pseudos branch from cbc1b71 to 5d0807b Compare May 30, 2026 09:52

edan-bainglass merged commit dc0ae7d into aiidalab:main May 30, 2026
6 of 7 checks passed

Uh oh!

Conversation

edan-bainglass commented May 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented May 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

edan-bainglass commented May 20, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

edan-bainglass commented May 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndresOrtegaGuerrero left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

edan-bainglass commented May 20, 2026 •

edited

Loading

codecov Bot commented May 20, 2026 •

edited

Loading

edan-bainglass commented May 30, 2026 •

edited

Loading