Add (optional) support for the AiiDA Gaussian Datatypes #104

dev-zero · 2020-04-21T18:00:25Z

No description provided.

yakutovicha

Thanks, @dev-zero, very nice work. I've made the first path through and I really liked it! I am not 100% sure if I understood everything, but if you can reply to my comments, things would get more clear. Essentially, the two big requests I have are:

Can you please provide a plain example that allows see how a user is supposed to define pseudo/basisset taking from the AiiDA Gaussian object (see my comment below).
Would it be possible to avoid the validate_basissets and validate_pseudos checks for the time being? I believe after we mature the API of Cp2kInput class (or replace it with cp2k-input-tools) they can be made way simpler and easier to maintain.

aiida_cp2k/calculations/__init__.py

aiida_cp2k/calculations/datatype_helpers.py

aiida_cp2k/calculations/__init__.py

test/test_gaussian_datatypes.py

yakutovicha · 2020-07-13T23:26:34Z

aiida_cp2k/calculations/datatype_helpers.py

+    return _validate_gdt_namespace(basissets, DataFactory("gaussian.basisset"), "basis set")
+
+
+def validate_basissets(inp, basissets, structure):


I see a lot of code duplication with the validate_pseudos function below. Can we merge them? It would make a lot of sence to me.

Also, I am tempted to suggest to not use this check. It appears to be quite a difficult one to maintain. Let's maybe work a bit more on the Cp2kInput class to make it more robust and with a nice/powerful API. Once this is done, we introduce those checks back but we base them on the new Cp2kInput API. What do you think?

I see a lot of code duplication with the validate_pseudos function below. Can we merge them? It would make a lot of sence to me.

I actually tried it once (similar to the _validate_gdt_namespace), but I ended up with highly parametrized code which was barely readable. and not much less code. Which is why opted for some duplication in the end instead.

Also, I am tempted to suggest to not use this check. It appears to be quite a difficult one to maintain. Let's maybe work a bit more on the Cp2kInput class to make it more robust and with a nice/powerful API. Once this is done, we introduce those checks back but we base them on the new Cp2kInput API. What do you think?

I don't like it for several reasons:

for the fully automated case where the user does not specify a KIND section anymore we have to generate the missing input at some place, hence we need some mechanism for that. Side-note: This could be done in Cp2kInput (or the respective cp2k-input-tools), but is also something which could be refactored later.

for the complex manual assignment cases we should be validating to avoid bogus links in the provenance graph and ensure CP2K is picking up only the basissets/pseudos passed by AiiDA.

loosening constraints (as done by these checks) should always be possible without breaking existing user-side code, while the other way round is most often not. So I would rather like to have it more complex on the developer side for now but safer and easier for the user to do the right thing, rather than the other way round

or the fully automated case where the user does not specify a KIND section anymore we have to generate the missing input at some place, hence we need some mechanism for that. Side-note: This could be done in Cp2kInput (or the respective cp2k-input-tools), but is also something which could be refactored later.

Will it work for multiple FORCE_EVAL sections? That was my primary concern. See an example here

I definitely agree that such a check is important. I am just worried that it may limit the generality of the plugin. My secondary concern is that we may need to reimplement things when we move to the cp2k-input-tools. I didn't try it yet, but I believe you have a nice navigation mechanism there.

Will it work for multiple FORCE_EVAL sections? That was my primary concern. See an example here

In the "manual" mode (user specifies basisset/pseudo names explicitly and passes the respective objects as inputs): yes. Here the validation is:

for all KIND sections, see whether we have the basisset/pseudo in the basissets/pseudos inputs

for all passed basissets/pseudos verify that they have been referenced in a KIND section

In the fully automated mode: not yet. The difficulty is that we might have to parse the fragments and the types of the force_evals to figure out where to create a subsys/kind section and for which symbols, which is indeed better suited in cp2k-input-tools since there we can directly validate against the schema.

The problem is that without those checks, the user can essentially pass in whatever and it may or may not work depending on what is present on the target node, which gives a false sense of security and bogus provenance graphs at best, and calculations with unintended basissets/pseudos at worst.

Tests for multiple FORCE_EVAL added.

test/test_gaussian_datatypes.py

aiida_cp2k/calculations/datatype_helpers.py

dev-zero · 2020-08-31T11:25:46Z

@yakutovicha thanks for the review, I am working on the test fixtures now (and trying to figure out why tests now fail even though I didn't change the functionality). Can you please give feedback on whether the examples are clean enough and the move of _unpack?

yakutovicha · 2020-08-31T11:27:11Z

@yakutovicha thanks for the review, I am working on the test fixtures now (and trying to figure out why tests now fail even though I didn't change the functionality). Can you please give feedback on whether the examples are clean enough and the move of _unpack?

sure, I will do it by tomorrow morning.

yakutovicha · 2020-09-01T09:45:43Z

@dev-zero fyi: I am testing things right now.

dev-zero · 2020-09-01T09:52:57Z

@yakutovicha cool, thanks. The test failure should now be fixed as well.

examples/gaussian_datatypes/example_explicit.py

yakutovicha

Let's merge it! Thanks for the nice work @dev-zero!

oschuett · 2020-09-04T11:52:16Z

The dashboard test started failing after this PR. The error message says:

aiida.common.exceptions.MissingEntryPointError: Entry point 'gaussian.basisset' not found in group 'aiida.data'. Try running `reentry scan` to update the entry point cache.

However, we are already running reentry after installing cp2k-aiida.

yakutovicha · 2020-09-04T13:09:29Z

Thanks for the report Ole. I will fix that.

yakutovicha · 2020-09-04T13:14:52Z

@oschuett, I think #110 should fix the problem.

dev-zero force-pushed the feature/integrate-datatypes branch 2 times, most recently from 7fb5157 to 6871391 Compare April 22, 2020 16:39

yakutovicha requested changes Jul 14, 2020

View reviewed changes

utils: add parse_iter for Cp2kInput to efficiently traverse dict

1b83679

dev-zero force-pushed the feature/integrate-datatypes branch from 10a20af to 78a11ba Compare August 25, 2020 09:37

dev-zero added 6 commits August 31, 2020 11:11

add support for aiida-gaussian-datatypes and tracked basissets

cf7bff6

Docker: install gaussian-datatypes for testing

1b13b5f

datatype_helpers: factor out common methods

32e2bc6

add support for pseudos

41777b7

allow KIND-less input, fix some issues with custom-named pseudos

90f4669

examples: add gdt examples

c24fec9

dev-zero force-pushed the feature/integrate-datatypes branch from 78a11ba to c24fec9 Compare August 31, 2020 09:17

dev-zero added 2 commits August 31, 2020 11:23

move all imports to toplevel and disable pylint suppression

4620ab5

group gdt imports by phase/function rather than datatype

e6da3fb

tests: use fixtures for basissets & pseudos

db40db3

calculations: fix check for optional namespaces basissets and pseudos

5a9aafd

yakutovicha reviewed Sep 1, 2020

View reviewed changes

examples/gaussian_datatypes/example_explicit.py Show resolved Hide resolved

yakutovicha reviewed Sep 1, 2020

View reviewed changes

examples/gaussian_datatypes/example_explicit.py Show resolved Hide resolved

dev-zero force-pushed the feature/integrate-datatypes branch from d87273c to d3ed164 Compare September 2, 2020 12:01

dev-zero added 2 commits September 2, 2020 15:25

tests/gdts: add testcase with multiple FORCE_EVAL

600be6c

examples/gdt: handle case for already imported basis sets transparently

ddeca70

dev-zero force-pushed the feature/integrate-datatypes branch from d3ed164 to ddeca70 Compare September 2, 2020 13:25

yakutovicha self-requested a review September 2, 2020 13:45

yakutovicha approved these changes Sep 2, 2020

View reviewed changes

yakutovicha merged commit 7db3189 into aiidateam:develop Sep 2, 2020

dev-zero deleted the feature/integrate-datatypes branch September 3, 2020 12:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add (optional) support for the AiiDA Gaussian Datatypes #104

Add (optional) support for the AiiDA Gaussian Datatypes #104

dev-zero commented Apr 21, 2020 •

edited

Loading

yakutovicha left a comment •

edited

Loading

yakutovicha Jul 13, 2020

yakutovicha Jul 14, 2020

dev-zero Aug 31, 2020

dev-zero Aug 31, 2020

yakutovicha Sep 1, 2020 •

edited

Loading

yakutovicha Sep 1, 2020

dev-zero Sep 2, 2020 •

edited

Loading

dev-zero Sep 2, 2020

dev-zero commented Aug 31, 2020

yakutovicha commented Aug 31, 2020

yakutovicha commented Sep 1, 2020

dev-zero commented Sep 1, 2020

yakutovicha left a comment

oschuett commented Sep 4, 2020

yakutovicha commented Sep 4, 2020

yakutovicha commented Sep 4, 2020

		return _validate_gdt_namespace(basissets, DataFactory("gaussian.basisset"), "basis set")


		def validate_basissets(inp, basissets, structure):

Add (optional) support for the AiiDA Gaussian Datatypes #104

Add (optional) support for the AiiDA Gaussian Datatypes #104

Conversation

dev-zero commented Apr 21, 2020 • edited Loading

yakutovicha left a comment • edited Loading

Choose a reason for hiding this comment

yakutovicha Jul 13, 2020

Choose a reason for hiding this comment

yakutovicha Jul 14, 2020

Choose a reason for hiding this comment

dev-zero Aug 31, 2020

Choose a reason for hiding this comment

dev-zero Aug 31, 2020

Choose a reason for hiding this comment

yakutovicha Sep 1, 2020 • edited Loading

Choose a reason for hiding this comment

yakutovicha Sep 1, 2020

Choose a reason for hiding this comment

dev-zero Sep 2, 2020 • edited Loading

Choose a reason for hiding this comment

dev-zero Sep 2, 2020

Choose a reason for hiding this comment

dev-zero commented Aug 31, 2020

yakutovicha commented Aug 31, 2020

yakutovicha commented Sep 1, 2020

dev-zero commented Sep 1, 2020

yakutovicha left a comment

Choose a reason for hiding this comment

oschuett commented Sep 4, 2020

yakutovicha commented Sep 4, 2020

yakutovicha commented Sep 4, 2020

dev-zero commented Apr 21, 2020 •

edited

Loading

yakutovicha left a comment •

edited

Loading

yakutovicha Sep 1, 2020 •

edited

Loading

dev-zero Sep 2, 2020 •

edited

Loading