Feature/handling molecules #834

AndresOrtegaGuerrero · 2024-10-01T08:56:26Z

This feature handles #734

Include the option molecule for StructureData editor
Band structure calculation should be disabled
Cell Optimizatio not available if structure is molecule
kpoint_distance should be disable and force [1,1,1]
Realoading an old workflow should restore normal behavior of app
PdosCalculation Test

Modification to molecule editor

Cell option no available for Molecule

Kpoint_Distance disable for Molecules

New Workflow shuold restore behavior

codecov · 2024-10-01T08:58:09Z

Codecov Report

Attention: Patch coverage is 90.00000% with 5 lines in your changes missing coverage. Please review.

Project coverage is 68.12%. Comparing base (9503aed) to head (e302867).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/aiidalab_qe/app/configuration/advanced.py	88.46%	3 Missing ⚠️
src/aiidalab_qe/app/configuration/workflow.py	88.88%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #834      +/-   ##
==========================================
+ Coverage   67.87%   68.12%   +0.25%     
==========================================
  Files          50       50              
  Lines        4507     4555      +48     
==========================================
+ Hits         3059     3103      +44     
- Misses       1448     1452       +4

Flag	Coverage Δ
python-3.11	`68.12% <90.00%> (+0.25%)`	⬆️
python-3.9	`68.15% <90.00%> (+0.25%)`	⬆️

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.

AndresOrtegaGuerrero · 2024-10-01T09:23:39Z

For this PR We need #833 merged to get the PDOS peaks otherwise is all 0.0

AndresOrtegaGuerrero · 2024-10-01T09:24:55Z

@edan-bainglass I included you since it involves widgets and some behaviour of the app , so you can give your opinion for changes or things to consider

cpignedoli

tested in combination with #833
Automatic selection of periodicity will have to follow as a separate PR. The user can then override the automatic selection.

edan-bainglass

Okay, all good from me! Note that it will likely all change in my PR - not the behavior, but the implementation, due to the introduction of MVC.

One comment though - I find it odd that a control that modifies the app behavior (disable this, remove that) is somewhat "hidden" deep down (expand step, expand editors, switch tab...). I would consider elevating such controls. I am planning a layout redesign PR after my MVC PR. Should be a fun one!

edan-bainglass · 2024-10-02T04:59:10Z

src/aiidalab_qe/app/configuration/workflow.py

+    @tl.observe("input_structure")
+    def _on_input_structure_change(self, change):
+        """Update the relax type options based on the input structure."""


In my rework, I generally reserve _on_some_state_change methods as handlers of observations, as you do here. But this also means they themselves do not handle logic. So everything below the method header (including the docstring) moves to a more specific method, here maybe _update_relax_type, passing to is change["new"] (or change if more than "new" is required). This keeps the "controller" _on methods light and clear. Reading them effectively tells you what is happening in the app (e.g., on input_structure change). Note that in my design, such methods as _update_relax_type end up moving to the model. The controller triggers this so-called "business logic" via the model, keeping the controller light and dedicated only to reactions.

Just giving you a heads up of what to expect in my beast PR, so no worries on changing it here 🙂

Yeah, I know! that part of removing was something that i wanted to check with you , so for molecules we cannot allow the user to do Cell Optimization , however in ipywidgets the ToogleButtons , there is no way to disable one option .. so I opted to go my way around and remove that option for molecules, while making sure it should be present for 1d,2d, and 3d systems.

Right, I've encountered the limitation before. Would be nice in general if any option-selection widget allowed disabling certain options. Oh well...

So , i would rename as
@tl.observe("input_structure")
def _update_relax_type(self, change):

or keep the name of _on_input_structure_change name , since i need the traitlets to trigger this function ?

Again, I wouldn't change it at this stage. But if you did...

@tl.observe("input_structure") def _on_input_structure_change(self, change): self._update_relax_type(change["new"]) # or `change` if you need more from it def _update_relax_type(self, input_structure): # or change (see above) ...

Sometimes, the triggered handler (_on_<state>_change) might trigger several methods. This pattern makes it clear what does are. For example:

on structure change: do this() then do that() then finally do the last thing()

Easier to maintain. Also, this, that, etc. are effectively "business logic", so they are moved to the model. Then you get

on structure change: model.do_this() model.do_that() model.the_last_thing()

Exception to the rule: controllers can also modify the view directly. In such a case, the method would not go through the model.

Please don't let any of the above block your PR 🙏

ok i will keep the name , and i guess later we sit together for lazy loading :D

AndresOrtegaGuerrero · 2024-10-04T09:41:49Z

I have adapted the PDOS tab for molecules where is compulsory to use degauss

AndresOrtegaGuerrero · 2024-10-04T09:44:25Z

I will wait for the input from @superstar54 or @PNOGillespie , to know there is no conflict with the XAS or XPS plugins. or extra modifications so is 100% compatible

superstar54

Hi @AndresOrtegaGuerrero , thanks for the nice work! I requested a few changes.

For the kpoints, I think we need to use gamma_only for molecule instead of [1, 1, 1], because the former one is faster.
XAS plugin does not support molecule calculation. I think @PNOGillespie could add a warning message in the XAS GUI if users activate the molecule option (in a future PR).
XPS plugin supports molecule, and has a molecule option in the XPS GUI explicitly, I can modify it in a future PR to use the molecule option from this PR.

So, I think your PR can go ahead without worry XAS and XPS.

src/aiidalab_qe/app/configuration/workflow.py

src/aiidalab_qe/app/configuration/__init__.py

superstar54 · 2024-10-04T10:14:36Z

src/aiidalab_qe/app/configuration/advanced.py

+    def create_kpoints_distance_link(self):
+        """Create the dlink for override and kpoints_distance."""
+        self.kpoints_distance_link = ipw.dlink(
+            (self.override, "value"),
+            (self.kpoints_distance, "disabled"),
+            lambda override: not override,
+        )
+
+    def remove_kpoints_distance_link(self):
+        """Remove the kpoints_distance_link."""
+        if hasattr(self, "kpoints_distance_link"):
+            self.kpoints_distance_link.unlink()
+            del self.kpoints_distance_link
+


I suggest not creating the link; instead, modify the _override_changed method. In this way, you only need to keep the logic in one place.

I need this link to be created and removed , so that the kpoints distance is not modify for molecules , while keeping the same logic for other periodicities

since is liked to the reset()

I understand that the link is used to disable the kpoints_distance widget. You can achieve this in _override_changed. If input_structure is a molecule, the kpoints_distance widget is disabled no matter the change is.

but the issue, is when uploading a job with pbc that is not molecule , so I tried with the override ,but i realize that since is linked with the reset , it was better to create and remove the link ,

AndresOrtegaGuerrero · 2024-10-04T10:43:38Z

Hi @AndresOrtegaGuerrero , thanks for the nice work! I requested a few changes.

For the kpoints, I think we need to use gamma_only for molecule instead of [1, 1, 1], because the former one is faster.

XAS plugin does not support molecule calculation. I think @PNOGillespie could add a warning message in the XAS GUI if users activate the molecule option (in a future PR).

XPS plugin supports molecule, and has a molecule option in the XPS GUI explicitly, I can modify it in a future PR to use the molecule option from this PR.

So, I think your PR can go ahead without worry XAS and XPS.

I would not use gamma_only , then there will be problems for vibroscopy app

superstar54 · 2024-10-04T10:50:10Z

there will be problems for vibroscopy app

gamma_only is one time faster than [1, 1, 1] and is suggested to be used in a normal case. The plugin could have its logic to handle the molecule case, e.g., the vibroscopy app can override the gamma_only to [1, 1,1].

Another parameter is assume_isolated; it's also suggested to use mt for the molecular system.

AndresOrtegaGuerrero · 2024-10-04T10:51:10Z

there will be problems for vibroscopy app

gamma_only is one time faster than [1, 1, 1] and is suggested to be used in a normal case. The plugin could have its logic to handle the molecule case, e.g., the vibroscopy app can override the gamma_only to [1, 1,1].

Another parameter is assume_isolated; it's also suggested to use mt for the molecular system.

The idea is to set this first and in a follow up set the MT , since this a change in the workchain ,

superstar54 · 2024-10-04T10:53:13Z

Ok to add MT in another PR.

btw, I can not find the gamma_only in the latest docs of pw, 😆

AndresOrtegaGuerrero · 2024-10-04T11:01:08Z

Ok to add MT in another PR.

btw, I can not find the gamma_only in the latest docs of pw, 😆

and the MT for the assume isolate ,
and logic for K_POINT{gamma} (And i think for this we will require a PR in aiida-quantumespresso)

Yest the best is to set that logic and how it affect other plugins in a follow up PR

superstar54

LGTM!

AndresOrtegaGuerrero added 4 commits September 27, 2024 16:06

init commit

d7986ed

widgets for molecule

0ed98f0

adding tests and fix widget behaviour

3f4d1ae

updating report

babed8a

AndresOrtegaGuerrero requested review from cpignedoli, superstar54 and edan-bainglass October 1, 2024 09:23

cpignedoli approved these changes Oct 1, 2024

View reviewed changes

edan-bainglass approved these changes Oct 2, 2024

View reviewed changes

AndresOrtegaGuerrero and others added 7 commits October 2, 2024 13:18

Merge branch 'main' into feature/handling_molecules

517fc29

Merge branch 'main' into feature/handling_molecules

081d637

Merge branch 'main' into feature/handling_molecules

75a9e32

Merge branch 'main' into feature/handling_molecules

ce1f927

update to main

3097a79

setting PDOS for molecules

2135569

Merge branch 'main' into feature/handling_molecules

0246168

AndresOrtegaGuerrero linked an issue Oct 4, 2024 that may be closed by this pull request

Handling Molecules #734

Closed

AndresOrtegaGuerrero added 2 commits October 4, 2024 10:04

test fix

894d0e3

test fix

e7e7026

superstar54 requested changes Oct 4, 2024

View reviewed changes

removing dedundancies

c56a3b3

AndresOrtegaGuerrero requested a review from superstar54 October 4, 2024 11:18

Merge branch 'main' into feature/handling_molecules

e302867

superstar54 approved these changes Oct 5, 2024

View reviewed changes

AndresOrtegaGuerrero merged commit 860f59b into main Oct 5, 2024
12 checks passed

AndresOrtegaGuerrero deleted the feature/handling_molecules branch October 5, 2024 06:48

AndresOrtegaGuerrero mentioned this pull request Oct 5, 2024

Gamma Point and Martyna-Tuckerman for molecules #841

Open

edan-bainglass added a commit to edan-bainglass/aiidalab-qe that referenced this pull request Oct 24, 2024

Handle aiidalab#834

1dbebc4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/handling molecules #834

Feature/handling molecules #834

AndresOrtegaGuerrero commented Oct 1, 2024 •

edited by edan-bainglass

Loading

codecov bot commented Oct 1, 2024 •

edited

Loading

AndresOrtegaGuerrero commented Oct 1, 2024

AndresOrtegaGuerrero commented Oct 1, 2024

cpignedoli left a comment

edan-bainglass left a comment

edan-bainglass Oct 2, 2024

edan-bainglass Oct 2, 2024

AndresOrtegaGuerrero Oct 2, 2024

edan-bainglass Oct 2, 2024

AndresOrtegaGuerrero Oct 2, 2024

edan-bainglass Oct 2, 2024

AndresOrtegaGuerrero Oct 2, 2024

AndresOrtegaGuerrero commented Oct 4, 2024

AndresOrtegaGuerrero commented Oct 4, 2024

superstar54 left a comment

superstar54 Oct 4, 2024

AndresOrtegaGuerrero Oct 4, 2024 •

edited

Loading

AndresOrtegaGuerrero Oct 4, 2024

superstar54 Oct 4, 2024

AndresOrtegaGuerrero Oct 4, 2024

AndresOrtegaGuerrero commented Oct 4, 2024

superstar54 commented Oct 4, 2024

AndresOrtegaGuerrero commented Oct 4, 2024 •

edited

Loading

superstar54 commented Oct 4, 2024

AndresOrtegaGuerrero commented Oct 4, 2024 •

edited

Loading

superstar54 left a comment

Feature/handling molecules #834

Feature/handling molecules #834

Conversation

AndresOrtegaGuerrero commented Oct 1, 2024 • edited by edan-bainglass Loading

codecov bot commented Oct 1, 2024 • edited Loading

Codecov Report

AndresOrtegaGuerrero commented Oct 1, 2024

AndresOrtegaGuerrero commented Oct 1, 2024

cpignedoli left a comment

Choose a reason for hiding this comment

edan-bainglass 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

AndresOrtegaGuerrero commented Oct 4, 2024

AndresOrtegaGuerrero commented Oct 4, 2024

superstar54 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndresOrtegaGuerrero Oct 4, 2024 • 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 commented Oct 4, 2024

superstar54 commented Oct 4, 2024

AndresOrtegaGuerrero commented Oct 4, 2024 • edited Loading

superstar54 commented Oct 4, 2024

AndresOrtegaGuerrero commented Oct 4, 2024 • edited Loading

superstar54 left a comment

Choose a reason for hiding this comment

AndresOrtegaGuerrero commented Oct 1, 2024 •

edited by edan-bainglass

Loading

codecov bot commented Oct 1, 2024 •

edited

Loading

AndresOrtegaGuerrero Oct 4, 2024 •

edited

Loading

AndresOrtegaGuerrero commented Oct 4, 2024 •

edited

Loading

AndresOrtegaGuerrero commented Oct 4, 2024 •

edited

Loading