Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/handling molecules #834

Merged
merged 15 commits into from
Oct 5, 2024
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions src/aiidalab_qe/app/configuration/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,11 @@ def __init__(self, **kwargs):
(self.advanced_settings, "input_structure"),
)
#
ipw.dlink(
(self, "input_structure"),
(self.workchain_settings, "input_structure"),
)
#
self.built_in_settings = [
self.workchain_settings,
self.advanced_settings,
Expand Down Expand Up @@ -113,6 +118,11 @@ def __init__(self, **kwargs):
def _observe_previous_step_state(self, _change):
self._update_state()

@tl.observe("input_structure")
def _observe_input_structure(self, _change):
if _change["new"] is not None and _change["new"].pbc == (False, False, False):
self.workchain_settings._on_input_structure_change(_change)

AndresOrtegaGuerrero marked this conversation as resolved.
Show resolved Hide resolved
def get_configuration_parameters(self):
"""Get the parameters of the configuration step."""

Expand Down
45 changes: 39 additions & 6 deletions src/aiidalab_qe/app/configuration/advanced.py
Original file line number Diff line number Diff line change
Expand Up @@ -107,11 +107,7 @@ def __init__(self, default_protocol=None, **kwargs):
style={"description_width": "initial"},
)
self.mesh_grid = ipw.HTML()
ipw.dlink(
(self.override, "value"),
(self.kpoints_distance, "disabled"),
lambda override: not override,
)
self.create_kpoints_distance_link()
self.kpoints_distance.observe(self._callback_value_set, "value")

# Hubbard setting widget
Expand Down Expand Up @@ -285,6 +281,20 @@ def __init__(self, default_protocol=None, **kwargs):
# Default settings to trigger the callback
self.reset()

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

Comment on lines +284 to +297
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@AndresOrtegaGuerrero AndresOrtegaGuerrero Oct 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since is liked to the reset()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ,

def _create_electron_maxstep_widgets(self):
self.electron_maxstep = ipw.BoundedIntText(
min=20,
Expand Down Expand Up @@ -332,10 +342,22 @@ def _update_input_structure(self, change):
self.hubbard_widget.update_widgets(change["new"])
if isinstance(self.input_structure, HubbardStructureData):
self.override.value = True
if self.input_structure.pbc == (False, False, False):
self.kpoints_distance.value = 100.0
self.kpoints_distance.disabled = True
if hasattr(self, "kpoints_distance_link"):
self.remove_kpoints_distance_link()
else:
# self.kpoints_distance.disabled = False
if not hasattr(self, "kpoints_distance_link"):
self.create_kpoints_distance_link()
else:
self.magnetization.input_structure = None
self.pseudo_setter.structure = None
self.hubbard_widget.update_widgets(None)
self.kpoints_distance.disabled = False
if not hasattr(self, "kpoints_distance_link"):
self.create_kpoints_distance_link()

@tl.observe("electronic_type")
def _electronic_type_changed(self, change):
Expand All @@ -356,7 +378,14 @@ def _update_settings_from_protocol(self, protocol):

parameters = PwBaseWorkChain.get_protocol_inputs(protocol)

self.kpoints_distance.value = parameters["kpoints_distance"]
if self.input_structure:
if self.input_structure.pbc == (False, False, False):
self.kpoints_distance.value = 100.0
self.kpoints_distance.disabled = True
else:
self.kpoints_distance.value = parameters["kpoints_distance"]
else:
self.kpoints_distance.value = parameters["kpoints_distance"]

num_atoms = len(self.input_structure.sites) if self.input_structure else 1

Expand Down Expand Up @@ -630,6 +659,10 @@ def reset(self):
self.pseudo_setter._reset()
else:
self.pseudo_setter._reset()
if self.input_structure.pbc == (False, False, False):
self.kpoints_distance.value = 100.0
self.kpoints_distance.disabled = True

# reset the magnetization
self.magnetization.reset()
# reset the hubbard widget
Expand Down
53 changes: 53 additions & 0 deletions src/aiidalab_qe/app/configuration/workflow.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@
"""

import ipywidgets as ipw
import traitlets as tl

from aiida import orm
from aiida_quantumespresso.common.types import RelaxType
from aiidalab_qe.app.parameters import DEFAULT_PARAMETERS
from aiidalab_qe.app.utils import get_entry_items
Expand Down Expand Up @@ -49,6 +51,8 @@ class WorkChainSettings(Panel):
with less precision and the "precise" protocol to aim at best accuracy (at the price of longer/costlier calculations).</div>"""
)

input_structure = tl.Instance(orm.StructureData, allow_none=True)

def __init__(self, **kwargs):
# RelaxType: degrees of freedom in geometry optimization
self.relax_type = ipw.ToggleButtons(
Expand Down Expand Up @@ -141,6 +145,54 @@ def update_reminder_info(change, name=name):
**kwargs,
)

@tl.observe("input_structure")
def _on_input_structure_change(self, change):
"""Update the relax type options based on the input structure."""
Comment on lines +148 to +150
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🙏

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if change["new"] is not None and change["new"].pbc == (False, False, False):
# Update relax_type options for non-periodic boundary conditions
self.relax_type.options = [
("Structure as is", "none"),
("Atomic positions", "positions"),
]
# Ensure the value is in the options
if self.relax_type.value not in [
option[1] for option in self.relax_type.options
]:
self.relax_type.value = "positions"

self.properties["bands"].run.value = False
self.properties["bands"].run.disabled = True

elif change["new"] is not None and change["new"].pbc != (False, False, False):
# Update relax_type options for periodic boundary conditions
self.relax_type.options = [
("Structure as is", "none"),
("Atomic positions", "positions"),
("Full geometry", "positions_cell"),
]
# Ensure the value is in the options
if self.relax_type.value not in [
option[1] for option in self.relax_type.options
]:
self.relax_type.value = "positions_cell"

self.properties["bands"].run.disabled = False

else:
# Default options when input_structure is None or other condition
self.relax_type.options = [
("Structure as is", "none"),
("Atomic positions", "positions"),
("Full geometry", "positions_cell"),
]
# Ensure the value is in the options
if self.relax_type.value not in [
option[1] for option in self.relax_type.options
]:
self.relax_type.value = "positions_cell"

self.properties["bands"].run.disabled = False
AndresOrtegaGuerrero marked this conversation as resolved.
Show resolved Hide resolved

def get_panel_value(self):
# Work chain settings
relax_type = self.relax_type.value
Expand Down Expand Up @@ -192,6 +244,7 @@ def set_panel_value(self, parameters):

def reset(self):
"""Reset the panel to the default value."""
self.input_structure = None
for key in ["relax_type", "spin_type", "electronic_type"]:
getattr(self, key).value = DEFAULT_PARAMETERS["workchain"][key]
self.workchain_protocol.value = DEFAULT_PARAMETERS["workchain"]["protocol"]
Expand Down
1 change: 1 addition & 0 deletions src/aiidalab_qe/app/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,7 @@ def _observe_process_selection(self, change):
self.structure_step.structure = process.inputs.structure
self.structure_step.confirm()
self.submit_step.process = process

# set ui_parameters
# print out error message if yaml format ui_parameters is not reachable
ui_parameters = process.base.extras.get("ui_parameters", {})
Expand Down
1 change: 1 addition & 0 deletions src/aiidalab_qe/app/result/summary_viewer.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
(True, True, True): "xyz",
(True, True, False): "xy",
(True, False, False): "x",
(False, False, False): "molecule",
}

VDW_CORRECTION_VERSION = {
Expand Down
7 changes: 2 additions & 5 deletions src/aiidalab_qe/common/widgets.py
Original file line number Diff line number Diff line change
Expand Up @@ -465,11 +465,7 @@ def __init__(self, title=""):
layout={"width": "initial"},
)
self.periodicity = ipw.RadioButtons(
options=[
"xyz",
"xy",
"x",
],
options=["xyz", "xy", "x", "molecule"],
value="xyz",
description="Periodicty: ",
layout={"width": "initial"},
Expand Down Expand Up @@ -613,6 +609,7 @@ def _select_periodicity(self, _=None):
"xyz": (True, True, True),
"xy": (True, True, False),
"x": (True, False, False),
"molecule": (False, False, False),
}
new_structure = deepcopy(self.structure)
new_structure.set_pbc(periodicity_options[self.periodicity.value])
Expand Down
8 changes: 8 additions & 0 deletions src/aiidalab_qe/plugins/pdos/setting.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ def __init__(self, **kwargs):
self.description = ipw.HTML(
"""<div style="line-height: 140%; padding-top: 0px; padding-bottom: 5px">
By default, the tetrahedron method is used for PDOS calculation. If required you can apply Gaussian broadening with a custom degauss value.
<br>
For molecules and systems with localized orbitals, it is recommended to use a custom degauss value.
</div>"""
)
# nscf kpoints setting widget
Expand Down Expand Up @@ -87,6 +89,12 @@ def _procotol_changed(self, change):
@tl.observe("input_structure")
def _update_structure(self, _=None):
self._display_mesh()
# For molecules this is compulsory
if self.input_structure.pbc == (False, False, False):
self.nscf_kpoints_distance.value = 100
self.nscf_kpoints_distance.disabled = True
self.use_pdos_degauss.value = True
self.use_pdos_degauss.disabled = True

def _display_mesh(self, _=None):
if self.input_structure is None:
Expand Down
37 changes: 37 additions & 0 deletions tests/configuration/test_advanced.py
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,43 @@ def test_advanced_kpoints_settings():
assert w.value.get("kpoints_distance") == 0.5


def test_advanced_molecule_settings(generate_structure_data):
"""Test kpoint setting of advanced setting widget."""
from aiidalab_qe.app.configuration.advanced import AdvancedSettings

w = AdvancedSettings()

# Check the disable of is bind to override switch
assert w.kpoints_distance.disabled is True

w.override.value = True
assert w.kpoints_distance.disabled is False

# create molecule
structure = generate_structure_data(name="H2O", pbc=(False, False, False))
# Assign the molecule
w.input_structure = structure

# Check override can not modify the kpoints_distance
assert w.kpoints_distance.disabled is True
w.override.value = True
assert w.kpoints_distance.disabled is True

# Confirm the value of kpoints_distance is fixed
assert w.value.get("kpoints_distance") == 100.0

w.protocol = "fast"
assert w.value.get("kpoints_distance") == 100.0

# # Check reset
w.input_structure = None
w.reset()

# # the protocol will not be reset
assert w.protocol == "fast"
assert w.value.get("kpoints_distance") == 0.5


def test_advanced_tot_charge_settings():
"""Test TotCharge widget."""
from aiidalab_qe.app.configuration.advanced import AdvancedSettings
Expand Down
7 changes: 7 additions & 0 deletions tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,13 @@ def _generate_structure_data(name="silicon", pbc=(True, True, True)):
structure.append_atom(position=(1.6, 0.92, 8.47), symbols="S")
structure.append_atom(position=(1.6, 0.92, 11.6), symbols="S")

elif name == "H2O":
cell = [[10.0, 0.0, 0.0], [0.0, 10.0, 0.0], [0.0, 0.0, 10.0]]
structure = orm.StructureData(cell=cell)
structure.append_atom(position=(0.0, 0.0, 0.0), symbols="H")
structure.append_atom(position=(0.0, 0.0, 1.0), symbols="O")
structure.append_atom(position=(0.0, 1.0, 0.0), symbols="H")

structure.pbc = pbc

return structure
Expand Down
Loading