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

aiida-cp2k cli #105

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

aiida-cp2k cli #105

wants to merge 5 commits into from

Conversation

dev-zero
Copy link
Contributor

@dev-zero dev-zero commented Apr 23, 2020

$ aiida-cp2k data structure --help
Usage: aiida-cp2k data structure [OPTIONS] COMMAND [ARGS]...

  Commands to create and inspect `StructureData` nodes from CP2K input.

Options:
  -h, --help  Show this message and exit.

Commands:
  import  Import a `StructureData` from a CP2K input file.

$ aiida-cp2k data structure import -n ../../cp2k/cp2k/H2.inp 
Success: parsed structure with formula H2

@dev-zero dev-zero changed the title [WiP] aiida-cp2k cli [RFC] aiida-cp2k cli Apr 23, 2020
@dev-zero dev-zero force-pushed the feature/cli branch 2 times, most recently from e2a4eb6 to f74e6f7 Compare October 7, 2020 16:01
@dev-zero dev-zero changed the title [RFC] aiida-cp2k cli aiida-cp2k cli Oct 7, 2020
@dev-zero
Copy link
Contributor Author

dev-zero commented Oct 7, 2020

@yakutovicha what do you think?

... including a simple structure import implementation to import
structures from a CP2K input file via the cp2k-input-tools.
…cture.import entrypoint

AiiDA v1.6.1 got a new entrypoint for registering command line format importers, use that to make "verdi data structure import cp2k ..." possible
Copy link
Contributor

@yakutovicha yakutovicha left a comment

Choose a reason for hiding this comment

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

Thanks, @dev-zero. Looks very good.

The only thing that I would like to change is the workflow submission part. Currently, you added there a cp2k input parsing, which (I believe) shouldn't be there.

What I want to propose instead is to add the data/input_dict command that would create an input Dict object. Then, when launching work chains I would rather prefer to deal with the existing AiiDA objects.

default=1,
show_default=True,
help='The maximum number of machines (nodes) to use for the calculations.')

Copy link
Contributor

Choose a reason for hiding this comment

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

Please add the number of CPUs per node parameter.

@dev-zero
Copy link
Contributor Author

Thanks for the review.

The only thing that I would like to change is the workflow submission part. Currently, you added there a cp2k input parsing, which (I believe) shouldn't be there.

What I want to propose instead is to add the data/input_dict command that would create an input Dict object. Then, when launching work chains I would rather prefer to deal with the existing AiiDA objects.

Just to make sure I understand it, you mean that the user will do a 2-step process? Something like:

$ aiida-cp2k data input_dict "/path/to/my/cp2k.inp"
Successfully generated the CP2K Parameter<6789>
Successfully created StructureData<6790>
Successfully imported BasisSet<6791>
...
$ aiida-cp2k workflow base cp2k@myhost -p 6789 -s 6790

Or that the import part should be factored out into a separate function?

@yakutovicha
Copy link
Contributor

yakutovicha commented Apr 30, 2021

Just to make sure I understand it, you mean that the user will do a 2-step process?

Yes. I also wanted to suggest parsing different objects separately:

$ aiida-cp2k data input_dict "/path/to/my/cp2k.inp"
Successfully generated the CP2K Parameter<6789>

$ aiida-cp2k data structure "/path/to/my/cp2k.inp"
Successfully created StructureData<6790>

$ aiida-cp2k data basisset  "/path/to/my/cp2k.inp"
Successfully imported BasisSet<6791>

This would allow importing objects from the other file types: CIF, XYZ for structure, JSON, YAML for input dict, etc.

The submission part looks ok to me.

@dev-zero
Copy link
Contributor Author

dev-zero commented May 1, 2021

Just to make sure I understand it, you mean that the user will do a 2-step process?

Yes. I also wanted to suggest parsing different objects separately:

$ aiida-cp2k data input_dict "/path/to/my/cp2k.inp"
Successfully generated the CP2K Parameter<6789>

$ aiida-cp2k data structure "/path/to/my/cp2k.inp"
Successfully created StructureData<6790>

$ aiida-cp2k data basisset  "/path/to/my/cp2k.inp"
Successfully imported BasisSet<6791>

This would allow importing objects from the other file types: CIF, XYZ for structure, JSON, YAML for input dict, etc.

Ok, will do. But it would be still nice to have a single command to simply run any given CP2K input via AiiDA with AiiDA taking care of all the rest.

About the import interface:

  • should we do like aiida-cp2k data (structure|basisset|...) (import|list|dump) <file> or if we are only doing import aiida-cp2k data import (structure|basisset|...) <file> resp. aiida-cp2k import (structure|basisset|...) <file>?
  • should the import be a calcfunction in the sense that a singlefiledata is created and a calcfunction generates the resulting structuredata/parameterdata nodes? There was a similar discussion for the extended structuredata plans if I remember correctly.

@yakutovicha
Copy link
Contributor

But it would be still nice to have a single command to simply run any given CP2K input via AiiDA with AiiDA taking care of all the rest.

I agree

should the import be a calcfunction in the sense that a singlefiledata is created and a calcfunction generates the resulting structuredata/parameterdata nodes? There was a similar discussion for the extended structuredata plans if I remember correctly.

I would say yes.

Btw, is your parsing tool capable of reading files that are referenced in the CP2K input (like structure, basis sets)?

@yakutovicha
Copy link
Contributor

yakutovicha commented May 3, 2021

  • should we do like aiida-cp2k data (structure|basisset|...) (import|list|dump) <file> or if we are only doing import aiida-cp2k data import (structure|basisset|...) <file> resp. aiida-cp2k import (structure|basisset|...) <file>?

I would say that aiida-cp2k data (structure|basisset|...) (import|list|dump) <file> is too much to ask from the cp2k plugin. The data objects we use are quite general. It would be better to implement listing and dumping directly in AiiDA: verdi data dict list.

aiida-cp2k import (structure|basisset|...) <file> sounds good enough to me. Maybe adding a file format flag would be useful.

We can also ask ping users, to hear their opinion.

@dev-zero
Copy link
Contributor Author

dev-zero commented May 4, 2021

should the import be a calcfunction in the sense that a singlefiledata is created and a calcfunction generates the resulting structuredata/parameterdata nodes? There was a similar discussion for the extended structuredata plans if I remember correctly.

I would say yes.

Hmm, ok, that makes it a bit more involved.

Btw, is your parsing tool capable of reading files that are referenced in the CP2K input (like structure, basis sets)?

It can correctly parse &COORD sections, returning the coordinates including unit normalization (but no rescaling for SCALED):
https://github.com/cp2k/cp2k-input-tools/blob/561926485a51b5d62319a0e333e1929ef3de0db7/cp2k_input_tools/parser.py#L229-L234

&CELL, and all *_FILE references are simply returned as such (no further check whether they exist). Embedded &BASIS_SET or &PSEUDOPOTENTIAL are returned as list under the * key. All CP2K preprocessor instructions are recognized (e.g. @INCLUDE are resolved).

@yakutovicha
Copy link
Contributor

@dev-zero I see you posted the same comment twice. Is my input still needed? Or will you go ahead implementing the changes that we discussed?

@yakutovicha
Copy link
Contributor

@dev-zero, please let me know if you need any help here. I can try to take over some of the parts.

@yakutovicha yakutovicha changed the base branch from develop to master November 21, 2022 08:09
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