SIRIUS-CP2K protocol and workchain

[hmhoseini]

protocol and workchain for SIRIUS-CP2K

[sphuber]

Thanks @hmhoseini . I have been out of the loop and am not quite sure what the purpose of the CP2K-SIRIUS build is. Was this implementation run separately from the base CP2K implementation for the PBE verification study? Or does this have some other purpose? I am asking to try and figure out whether this implementation should be added as merely another protocol sirius to the CP2K protocols, or whether it should get its own independent implementation with a fast, moderate protocol etc.

Also, what is special about these pseudos? The look to be pseudos that also occur in the SSSP families. Is there a reason they cannot be used? If they have to be in JSON format for CP2K-SIRIUS to be able to use them, there is a upftojson tool written by the SIRIUS developers to make this possible.

@yakutovicha @bosonie could you fill me in a bit on the context here?

[hmhoseini]

@sphuber
The purpose is PBE verification study. I think it should be a CP2K protocol.
The potentials are SSSP, converted to json format by upftojson.

[yakutovicha]

@sphuber, indeed, I think this PR needs more work. The immediate change I would do is to remove all the pseudos and convert them to JSON format on the fly.

I am asking to try and figure out whether this implementation should be added as merely another protocol sirius to the CP2K protocols, or whether it should get its own independent implementation with a fast, moderate protocol etc

The generator and the workflow differ very little from the original CP2K ones. IMO, SIRIUS should be implemented as another protocol with some minor changes to the original CP2K builder.

[hmhoseini]

@yakutovicha
Regarding converting pseudopotentials on the fly, where would you store (or read from) UPF potentials?
The changes for CP2K-SIRIUS are small and necessary only for few sections like files, k-point, and kinds.

[yakutovicha]

@yakutovicha Regarding converting pseudopotentials on the fly, where would you store (or read from) UPF potentials?

You can use aiida-pseudo for that. See here for more details on how to use it.

[sphuber]

The immediate change I would do is to remove all the pseudos and convert them to JSON format on the fly.

I think this might have to be done in the Cp2kCalculation class then. It is there where the pseudo is written to the working directory. If you want to do the conversion in the workchain, you will hvae to create a new UpfData from the original one, changing the format from UPF to JSON, and then pass that new UpfData to the Cp2kBaseWorkChain which will pass it to Cp2kCalculation. But if you don't do this in a clever way, you will end up creating a clone of all UpfData inputs each time you run the workchain

[yakutovicha]

The immediate change I would do is to remove all the pseudos and convert them to JSON format on the fly.

I think this might have to be done in the Cp2kCalculation class then. It is there where the pseudo is written to the working directory. If you want to do the conversion in the workchain, you will hvae to create a new UpfData from the original one, changing the format from UPF to JSON, and then pass that new UpfData to the Cp2kBaseWorkChain which will pass it to Cp2kCalculation. But if you don't do this in a clever way, you will end up creating a clone of all UpfData inputs each time you run the workchain

I think this is a fair point. I can add an optional UpfData input to the CP2K calculation plugin 👍

[bosonie]

@yakutovicha @hmhoseini Hi there. Sorry to come back to this, but we need to make a release next week. The paper is already accepted and we are only waiting the final format. Can we finish up this PR by Tuesday? Thanks and let me know if I can do anything from my side

[yakutovicha]

@bosonie, I need to adapt the CP2K plugin for the PR to work. Considering my current workload, Thursday looks more realistic.

[bosonie]

Ok, thanks @yakutovicha

[bosonie]

Hi @yakutovicha, what is the state of this PR?

[yakutovicha]

Hi @yakutovicha, what is the state of this PR?

I started to work on it on Thu, but couldn't complete it then. Friday was off. So I am continuing today.

[yakutovicha]

The potentials are SSSP, converted to json format by upftojson.

@hmhoseini, which exact version of the pseudopotentials did you use?

[hmhoseini]

@yakutovicha
SSSP-prec-v1.2

[yakutovicha]

For the information: the corresponding feature is implemented in the aiida-cp2k plugin and backported to aiida-cp2k-1.x.

[yakutovicha]

Looks like it is done now. I have a few minor points before we can conclude.

@bosonie we named the sirius protocol as sirius. However, such a name is not allowed from the CLI. Do you have any suggestions on what we should do here?

@hmhoseini, could you please test the changes if they work fine for you as well? Especially, the part that is responsible for UKS calculations:

        # Magnetic calculation.
        if spin_type == SpinType.NONE:
            parameters['FORCE_EVAL']['DFT']['UKS'] = False
            if magnetization_per_site is not None:
                import warnings
                warnings.warn('`magnetization_per_site` will be ignored as `spin_type` is set to SpinType.NONE')
        elif spin_type == SpinType.COLLINEAR:
            parameters['FORCE_EVAL']['DFT']['UKS'] = True
            structure, magnetization_tags = tags_and_magnetization(structure, magnetization_per_site)
            parameters['FORCE_EVAL']['DFT']['MULTIPLICITY'] = guess_multiplicity(structure, magnetization_per_site)

[bosonie]

@yakutovicha thanks a million! Really appreciate your effort. I will take care myself of the CLI problem (already PR open #315). However I have a suggestion. I would call the protocol verification-PBE-v1-sirius so to leave the possibility to have more sirius protocols in the future. What do you think?

[yakutovicha]

@yakutovicha thanks a million! Really appreciate your effort. I will take care myself of the CLI problem (already PR open #315).

perfect, thanks!

However I have a suggestion. I would call the protocol verification-PBE-v1-sirius so to leave the possibility to have more sirius protocols in the future. What do you think?

Sure, done in e325cca

[hmhoseini]

Looks like it is done now. I have a few minor points before we can conclude.

@bosonie we named the sirius protocol as sirius. However, such a name is not allowed from the CLI. Do you have any suggestions on what we should do here?

@hmhoseini, could you please test the changes if they work fine for you as well? Especially, the part that is responsible for UKS calculations:

        # Magnetic calculation.
        if spin_type == SpinType.NONE:
            parameters['FORCE_EVAL']['DFT']['UKS'] = False
            if magnetization_per_site is not None:
                import warnings
                warnings.warn('`magnetization_per_site` will be ignored as `spin_type` is set to SpinType.NONE')
        elif spin_type == SpinType.COLLINEAR:
            parameters['FORCE_EVAL']['DFT']['UKS'] = True
            structure, magnetization_tags = tags_and_magnetization(structure, magnetization_per_site)
            parameters['FORCE_EVAL']['DFT']['MULTIPLICITY'] = guess_multiplicity(structure, magnetization_per_site)

@yakutovicha Could you specify the version of aiida-core and aiida-cp2k that work well for you?

[yakutovicha]

@yakutovicha Could you specify the version of aiida-core and aiida-cp2k that work well for you?

Yes. You can use aiida-core 1.6.7 and aiida-cp2k 1.6.0

[hmhoseini]

@yakutovicha Could you specify the version of aiida-core and aiida-cp2k that work well for you?

Yes. You can use aiida-core 1.6.7 and aiida-cp2k 1.6.0

I tested the changes (few unaries and few oxides). Everything is fine except that spin-polarized calculation is not implemented for PW-DFT. That is, "UKS True" has no effect on SIRIUS PW-DFT calculations.

[yakutovicha]

I tested the changes (few unaries and few oxides). Everything is fine except that spin-polarized calculation is not implemented for PW-DFT. That is, "UKS True" has no effect on SIRIUS PW-DFT calculations.

Since it has no effect should we keep it as is, or disable that part for sirius protocol?

[hmhoseini]

I tested the changes (few unaries and few oxides). Everything is fine except that spin-polarized calculation is not implemented for PW-DFT. That is, "UKS True" has no effect on SIRIUS PW-DFT calculations.

Since it has no effect should we keep it as is, or disable that part for sirius protocol?

I suggest to disable "SpinType.COLLINEAR" and to make "SpinType.NONE" the default for sirius protocol .

[yakutovicha]

I suggest to disable "SpinType.COLLINEAR" and to make "SpinType.NONE" the default for sirius protocol .

Okay, since UKS is FALSE by default, I simply moved that section under if 'sirius' not in protocol: block. It should have the same effect.

@hmhoseini do you mind testing it again really quick? Just to make sure there are no stupid problems. I guess we can then merge.

[hmhoseini]

I suggest to disable "SpinType.COLLINEAR" and to make "SpinType.NONE" the default for sirius protocol .

Okay, since UKS is FALSE by default, I simply moved that section under if 'sirius' not in protocol: block. It should have the same effect.

@hmhoseini do you mind testing it again really quick? Just to make sure there are no stupid problems. I guess we can then merge.

I tested "verification-pbe-v1" and "verification-pbe-v1-sirius" protocols with "SpinType.COLLINEAR" and "SpinType.NONE" options.
It worked fine.

[yakutovicha]

I tested "verification-pbe-v1" and "verification-pbe-v1-sirius" protocols with "SpinType.COLLINEAR" and "SpinType.NONE" options.
It worked fine.

Great, I think we can merge it then. I am going to approve. I let @bosonie do the merge.

[sphuber]

Thanks @hmhoseini @yakutovicha