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

Set relax type none and no properites to run SCF only #274

Merged
merged 5 commits into from
Nov 16, 2022

Conversation

unkcpz
Copy link
Member

@unkcpz unkcpz commented Sep 1, 2022

fixes #264

Using RelaxType.NONE for relax workchain to run the SCF only.
The builder is constructed with all three workchain input parameters port, relax, bands, pdos.
Whether to run sub-workchain is controlled by popping the input key out from the builder, right after the builder is created.
Also, the relax/properties infos of workchain selector is changed to show this newly add calculation type as 'SCF only'.

@unkcpz unkcpz requested a review from mbercx September 1, 2022 22:22
@unkcpz unkcpz requested review from wengmouyi and mbercx and removed request for mbercx September 1, 2022 22:44
@unkcpz unkcpz removed the request for review from wengmouyi October 11, 2022 09:43
@unkcpz unkcpz added this to the AiiDA 2.x milestone Nov 7, 2022
@superstar54
Copy link
Member

One comment, after the SCF calculation, what results could the user get? Currently, the energy and force are not printed out in the result section. Better to implement this also. Otherwise, the calculation is meaningless.

@superstar54 superstar54 self-requested a review November 7, 2022 11:01
@superstar54
Copy link
Member

It is also confusing that one runs a SCF calculation using the relax port. Better to add a SCF workchain, to make the code clean and readable.

@unkcpz
Copy link
Member Author

unkcpz commented Nov 7, 2022

Thanks! @superstar54

after the SCF calculation, what results could the user get? Currently, the energy and force are not printed out in the result section. Better to implement this also. Otherwise, the calculation is meaningless.

This is a very good point. But this issue came from the in-person testing with Nicola that when selecting not relax and no properties are chosen the qeApp will block the calculation from running by throwing a warning.
Is that possible to leave it in the future since we do not decide yet what to show as the output of the SCF only calculation? I'll open an issue for it.

It is also confusing that one runs a SCF calculation using the relax port. Better to add a SCF workchain, to make the code clean and readable.

I also had same feeling when start working on this. But qeApp is simply the UI wrapper of aiida-quantumespresso, I think the change should be make there. @mbercx can you comment on if you think this is worth implementing?
I think we can basically add an alias workchain called PwScfOnlyWorkChain sort of, and have it point to Relax with the parameters fixed. The question is where to put this workchain? If it is not a case to add in aiida-quantumespresso, I think it is also make sense to have the workchain here in QeApp/src. I'll add it in this PR.

@unkcpz
Copy link
Member Author

unkcpz commented Nov 7, 2022

Hi @superstar54, I change the message show in workflow bar. But I still not add the energies showed in the output tabs. Since I don't decide yet where to add it, it can be on in the Workflow summary or as a new tab with all related and necessary output parameters. I open an issue at #311.
Let's focus on it with another PR.

@mbercx
Copy link
Member

mbercx commented Nov 7, 2022

Re the SCF only output: I think one of the possible outputs Nicola had in mind would be the charge density. However, I don't think it's possible to extract the charge density file atm. I'll add that originally we blocked this type of calculation for this very reason: I saw little point in running only an SCF for a layman user. The total energy of a single calculation is also not super meaningful, I'd say? The forces could be useful in some cases, I suppose.

Re the relax work chain: An "SCF" work chain would simply be the PwBaseWorkChain, no? The RelaxType.NONE implementation comes from the common workflows project, there it was decided to add this relax type for technical reasons, and here we simply repurpose this to make the implementation somewhat easier. I'd have to take a closer look at the code (which I'll try to make some time for ASAP, many apologies @unkcpz!), but I think you'd also have to make quite some significant changes to the QeAppWorkChain to add this to a proper (optional) port. Not sure if the resulting code would be easier to maintain.

@unkcpz
Copy link
Member Author

unkcpz commented Nov 10, 2022

@mbercx Thanks! I update the #311 with your comment. I think for the current one, it is okay to use the PwRelaxWorkChain with RelaxType.None. Since in my opinion, we should not add complexity to the workchain of QeApp but just light wrapper of what already implement in the aiida-quantumespresso.

@superstar54 could you have another look and let's try get this PR done?

aiidalab_qe/process.py Outdated Show resolved Hide resolved
builder_parameters = workchain.get_extra("builder_parameters", {})
relax_type = builder_parameters.get("relax_type")

if relax_type != "none":
Copy link
Member

Choose a reason for hiding this comment

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

Why is "none", not None or "None". Please make sure "none" is correct.

If it is correct, we need to change it to a more standard way, e.g. None

Copy link
Member Author

Choose a reason for hiding this comment

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

relax_type is a parameter read from widget.

        # RelaxType: degrees of freedom in geometry optimization
        self.relax_type = ipw.ToggleButtons(
            options=[
                ("Structure as is", "none"),
                ("Atomic positions", "positions"),
                ("Full geometry", "positions_cell"),
            ],
            value="positions_cell",
        )

The string type is used to distinguish the None in purpose.

@@ -98,27 +99,27 @@ def _generate_report_dict(qeapp_wc):
# read default from protocol
smearing = default_params["smearing"]

if run_relax:
try:
Copy link
Member

Choose a reason for hiding this comment

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

I think this try ... except is not used, because it will always run a relax (scf or real relax) calculation.

@@ -878,6 +865,12 @@ def update_builder(buildy, resources, npools):
if "smearing_override" in parameters:
builder.smearing_override = Str(parameters["smearing_override"])

# skip relax sub-worflow only when RelaxType is NONE and has property calculated.
Copy link
Member

Choose a reason for hiding this comment

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

Could you explain the difference between relax (only scf) and relax sub-workflow? I am not very clear about this, thanks.

Copy link
Member Author

Choose a reason for hiding this comment

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

aiidalab_qe/steps.py Outdated Show resolved Hide resolved
Copy link
Member

@superstar54 superstar54 left a comment

Choose a reason for hiding this comment

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

LGTM

@unkcpz unkcpz merged commit e8197eb into master Nov 16, 2022
@unkcpz unkcpz deleted the fix/264/scf-only branch November 16, 2022 21:25
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.

Allowed to run SCF calculation only
3 participants