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

run_relax should be used for get pw-parameters #319

Merged
merged 2 commits into from
Nov 24, 2022
Merged

run_relax should be used for get pw-parameters #319

merged 2 commits into from
Nov 24, 2022

Conversation

unkcpz
Copy link
Member

@unkcpz unkcpz commented Nov 23, 2022

fixes #318

@superstar54 We had a discussion about this, seems I didn't change these parts of code back to normal.

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.

There is a logic bug here.

If we run scf only, the pw_parameters will be None. Thus, the following code will raise a error.

    energy_cutoff_wfc = round(pw_parameters["SYSTEM"]["ecutwfc"])

Since we use parameters["relax_type"]=RealxType.NONE to run the scf calculation. So, no matter what kind of calculations we choose, there should always have a relax calculation. Therefore, line 33

run_relax = builder_parameters.get("relax_type") != "none"

needs change to:

run_relax = True

In file steps.py, line 871,

        # skip relax sub-workflow only when RelaxType is NONE and has property calculated.
        if RelaxType(parameters["relax_type"]) is RelaxType.NONE and (
            parameters["run_bands"] or parameters["run_pdos"]
        ):
            builder.pop("relax")

The above code should be removed, to make sure there is a relax calculation.

@unkcpz What do you think?

@unkcpz
Copy link
Member Author

unkcpz commented Nov 24, 2022

Hi @superstar54, the issue is more complex. Please check the comment I add to the code. The problem all comes from us running SCF which is a non-relax calculation using the PwRelaxWorkChain.

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.

@unkcpz You are right. One quick solution would be:
Change

    run_relax = builder_parameters.get("relax_type") != "none"
    run_bands = builder_parameters.get("run_bands")
    run_pdos = builder_parameters.get("run_pdos")

to:

    run_bands = builder_parameters.get("run_bands")
    run_pdos = builder_parameters.get("run_pdos")
    run_relax = not (builder_parameters.get("relax_type") == "none" & (run_relax or run_pdos))

@unkcpz
Copy link
Member Author

unkcpz commented Nov 24, 2022

@superstar54 thanks! I implement it by check if the relax port is in the workchain, please see my implementation, what you suggest has the advantage that the change all kept in one place, but to me, it is still very mind-binding.

    # The run_relax variable is not same as if statement below.
    # the "relax" port is poped out to skip the real relaxiation
    # which is not the case of SCF, we use the relax workchain but with
    # relax_type set to none as SCF calculation.
    if "relax" in qeapp_wc.inputs:
        pw_parameters = qeapp_wc.inputs.relax.base.pw.parameters.get_dict()
        if scf_kpoints_distance is None:
            scf_kpoints_distance = qeapp_wc.inputs.relax.base.kpoints_distance.value

I run two tests, one SCF and one relax=none + property. All works fine.

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!

@superstar54 superstar54 self-requested a review November 24, 2022 20:48
@unkcpz
Copy link
Member Author

unkcpz commented Nov 24, 2022

@superstar54 thanks millions!

@unkcpz unkcpz merged commit 2aab0ce into master Nov 24, 2022
@unkcpz unkcpz deleted the fix/318 branch November 24, 2022 20:53
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.

Does not have an input with link label 'relax' in report widget
2 participants