Skip to content

Adding relax workflow for ABACUS - #349

Open
zhubonan wants to merge 11 commits into
aiidateam:masterfrom
MCresearch:feature/abacus
Open

Adding relax workflow for ABACUS#349
zhubonan wants to merge 11 commits into
aiidateam:masterfrom
MCresearch:feature/abacus

Conversation

@zhubonan

Copy link
Copy Markdown
Collaborator

This PR adds the relaxation workflow for ABACUS which has been used to generate the contributed verification data on the Materials Cloud (https://archive.materialscloud.org/record/2025.151).

@t-reents t-reents left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hi @zhubonan
Sorry for checking it only now, but I just saw it by chance (since only @giovannipizzi was tagged for a review).
First of all, thanks again for your contribution!

I checked the PR and there are a few things to discuss/adapt, see my comments below. Let me know if you have questions and my comments are not clear

from ..workchain import CommonRelaxWorkChain
from .generator import AbacusCommonRelaxInputGenerator

__all__ = ('AbacusCommonRelaxInputGenerator',)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This should be the following, no?

Suggested change
__all__ = ('AbacusCommonRelaxInputGenerator',)
__all__ = ('AbacusCommonRelaxWorkChain',)

Comment on lines +17 to +33
# Default to use the free energy (E_KohnSham - TS) since it is consistent with the
# forces and is also used by the ACWF verification project
total_energy = misc.base.attributes.get('total_energy')
total_magnetization = misc.base.attributes.get('total_magnetization', None)

results = {'total_energy': orm.Float(total_energy)} # The reported energy is eV

# Get the forces and stress, abacus report these values in eV/A and KBar
forces = np.array(misc['final_forces'])
forces = orm.ArrayData({'forces': forces})
stress = np.array(misc['final_stress']) / 10 # Convert to GPa
stress = orm.ArrayData({'stress': stress})

if total_magnetization is not None:
results['total_magnetization'] = orm.Float(total_magnetization)

return {'total_energy': orm.Float(total_energy), 'forces': forces, 'stress': stress}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The results dict is created and updated with the total_magnetization but never used. A new "unrelated" dict is returned, which also misses the magnetic information

Comment thread abacus.py
@@ -0,0 +1,60 @@
from aiida import orm

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I guess this file should be entirely removed, right?



def get_ts_energy(common_relax_workchain: AbacusCommonRelaxWorkChain) -> float:
"""Return the T * S value of a concluded ``QuantumEspressoCommonRelaxWorkChain``.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Doc-string needs to be updated

# else:
# initial_magnetic_moments = dict(kind_to_magnetization)
# else:
initial_magnetic_moments = None

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is magnetism not supported in abacus? According to the documentation, it does? https://aiida-abacus.readthedocs.io/en/latest/howto/calculations.html#id2

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think that the magnetization shouldn't be silently ignored

@@ -0,0 +1,56 @@
"""Implementation of `aiida_common_workflows.common.relax.workchain.CommonRelaxWorkChain` for Quantum ESPRESSO."""

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Fix doc-string



class AbacusCommonRelaxWorkChain(CommonRelaxWorkChain):
"""Implementation of `aiida_common_workflows.common.relax.workchain.CommonRelaxWorkChain` for Quantum ESPRESSO."""

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Fix doc-string

@@ -0,0 +1,29 @@
"""
Post-processing extractor functions for the ``QuantumEspressoCommonRelaxWorkChain``.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Fix doc-string

The ports defined on the specification are the inputs that will be accepted by the ``get_builder`` method.
"""
super().define(spec)
spec.inputs['protocol'].valid_type = ChoiceType(('fast', 'moderate', 'precise', 'verification-PBE-v1'))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is overwritten in the next line

Suggested change
spec.inputs['protocol'].valid_type = ChoiceType(('fast', 'moderate', 'precise', 'verification-PBE-v1'))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants