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

upf to json converter #3308

Merged
merged 5 commits into from
Oct 29, 2019
Merged

Conversation

simonpintarelli
Copy link
Contributor

  • _prepare_json to upf class
  • added export, e.g. verdi data upf export PK to cli interface

@yakutovicha
Copy link
Contributor

Thanks @simonpintarelli for the PR.

@giovannipizzi, @sphuber this export functionality is needed to enable SIRIUS support within the aiida-cp2k plugin. I think the same could be used to support SIRIUS under QE.

@yakutovicha yakutovicha self-requested a review September 11, 2019 12:48
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, @simonpintarelli ,

Before I start the review please add a couple of tests (upf1->json, upf2->json) and fix the problems raised by the pre-commit run. Just run pre-commit run --all-files to see all of them

As soon as it is done, I think there shouldn't be a problem to merge it.

@CasperWA
Copy link
Contributor

Before I start the review please add a couple of tests (upf1->json, upf2->json) and fix the problems raised by the pre-commit run. Just run pre-commit run --all-files to see all of them

Hi @simonpintarelli, I am unsure how familiar you are with pre-commit, so just to be more explicit (and if you already know it, then hopefully no harm done 😄 ):
To have pre-commit, AiiDA may be installed specifying 'dev_precommit', e.g.

pip install -U -e .[dev_precommit]

when in the folder of the git cloned 'aiida-core' folder, or

pip install -U aiida[dev_precommit]

if installing from PyPI and using AiiDA v0.12 (should not be the case here).

After this, still being in the git cloned 'aiida-core' folder, you can run

pre-commit install

and then, finally

pre-commit run --all-files

as @yakutovicha writes 😃

@simonpintarelli
Copy link
Contributor Author

@CasperWA, I did pip install pre-commit, but apparently forgot to run pre-commit install.

Copy link
Member

@giovannipizzi giovannipizzi left a comment

Choose a reason for hiding this comment

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

For now we could include this into AiiDA core, even if eventually we want to move materials-science related classes in a different repository/plugin.

One question: do you have in mind of preparing a minimal library to manage UPF and then you could keep the code there and just depend on it, or this is the whole code for UPF that you have so it's easier to add it to aiida_core verbatim, as you are doing in this PR?

json_string, _ = self.pseudo_oxygen_upf1._prepare_json()
filepath_base = os.path.abspath(os.path.join(__file__, os.pardir, os.pardir, os.pardir, 'fixtures', 'pseudos'))
reference_json = json.dumps(json.load(open(os.path.join(filepath_base, 'O_raw.json'), 'r')))
self.assertEqual(json_string, reference_json.encode('utf-8'))
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 compare the dictionaries rather than the strings?

@@ -0,0 +1,3 @@
""" UPF converter """

from .upf_to_json import upf_to_json
Copy link
Member

Choose a reason for hiding this comment

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

The right place for these tools (we're slowly moving also other things there) is not in aiida/orm/data but in aiida/tools/data/upf/.... Could you move the logic there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@giovannipizzi Yes, this is the entire code we have for UPF to json conversion. Although moving it to a separate dependency might be a good idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moving the files to aiida/tools/data/upf seems to trigger a circular import if I then import the function in orm/nodes/data/upf.py.

@simonpintarelli simonpintarelli force-pushed the upf-to-json branch 2 times, most recently from 81fbc7a to 6be3a57 Compare October 6, 2019 21:15
@simonpintarelli
Copy link
Contributor Author

I've moved the code to aiida/tools/data/upf/...

It passes through prospector on my computer and and export upf to json via verdi works as desired. For some reason now travis keeps failing on both python3 & 2:

Traceback (most recent call last):
  File "/home/travis/virtualenv/python3.6.3/bin/verdi", line 10, in <module>
    sys.exit(verdi())
  File "/home/travis/virtualenv/python3.6.3/lib/python3.6/site-packages/click/core.py", line 764, in __call__
    return self.main(*args, **kwargs)
  File "/home/travis/virtualenv/python3.6.3/lib/python3.6/site-packages/click/core.py", line 717, in main
    rv = self.invoke(ctx)
  File "/home/travis/virtualenv/python3.6.3/lib/python3.6/site-packages/click/core.py", line 1137, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/home/travis/virtualenv/python3.6.3/lib/python3.6/site-packages/click/core.py", line 956, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/home/travis/virtualenv/python3.6.3/lib/python3.6/site-packages/click/core.py", line 555, in invoke
    return callback(*args, **kwargs)
  File "/home/travis/virtualenv/python3.6.3/lib/python3.6/site-packages/aiida/cmdline/commands/cmd_setup.py", line 47, in setup
    from aiida import orm
  File "/home/travis/virtualenv/python3.6.3/lib/python3.6/site-packages/aiida/orm/__init__.py", line 22, in <module>
    from .nodes import *
  File "/home/travis/virtualenv/python3.6.3/lib/python3.6/site-packages/aiida/orm/nodes/__init__.py", line 16, in <module>
    from .data import *
  File "/home/travis/virtualenv/python3.6.3/lib/python3.6/site-packages/aiida/orm/nodes/data/__init__.py", line 32, in <module>
    from .upf import UpfData
  File "/home/travis/virtualenv/python3.6.3/lib/python3.6/site-packages/aiida/orm/nodes/data/upf.py", line 23, in <module>
    from aiida.tools.data.upf.to_json import upf_to_json
  File "/home/travis/virtualenv/python3.6.3/lib/python3.6/site-packages/aiida/tools/__init__.py", line 28, in <module>
    from .data.array.kpoints import *
  File "/home/travis/virtualenv/python3.6.3/lib/python3.6/site-packages/aiida/tools/data/array/kpoints/__init__.py", line 18, in <module>
    from aiida.orm import KpointsData, Dict
ImportError: cannot import name 'KpointsData'
The command ".ci/setup_profiles.sh" failed and exited with 1 during .

@simonpintarelli simonpintarelli force-pushed the upf-to-json branch 3 times, most recently from 78d6cfe to ac923d1 Compare October 9, 2019 15:07
@giovannipizzi
Copy link
Member

I have the feeling this might be a cyclic import error... can you investigate if you understand if this is the case, having this hint?

@simonpintarelli simonpintarelli force-pushed the upf-to-json branch 5 times, most recently from 5d67b4a to bc12816 Compare October 14, 2019 11:23
@simonpintarelli
Copy link
Contributor Author

@giovannipizzi There was a circular dependency, I've moved the json converter now to a separate package.
The unit tests for checking the generated json seem to pass now, there were roundoff errors, so the dictionaries need to be compared manually.

There is one failure remaining. Where do I have to add a generator of a dummy instance?

======================================================================
ERROR: test_data_exporters (aiida.backends.tests.orm.data.test_data.TestData)
Verify that the return value of the export methods of all `Data` sub classes have the correct type.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/aiidateam/aiida-core/aiida/backends/tests/orm/data/test_data.py", line 75, in test_data_exporters
    instance = self.generate_class_instance(data_class)
  File "/home/travis/build/aiidateam/aiida-core/aiida/backends/tests/orm/data/test_data.py", line 57, in generate_class_instance
    'for this data class, add a generator of a dummy instance here'.format(data_class)
RuntimeError: no instance generator implemented for class `<class 'aiida.orm.nodes.data.upf.UpfData'>`. If you have added a `_prepare_*` method for this data class, add a generator of a dummy instance here

@sphuber
Copy link
Contributor

sphuber commented Oct 14, 2019

You need to add a clause to aiida.backends.tests.orm.data.test_data.TestData.generate_class_instance class method. Add a clause if data_class is orm.UpfData where you return a UpfData instance. You can use one of the UPF files you use for testing the conversion between UPF and json formats as a dummy file.

@simonpintarelli
Copy link
Contributor Author

@sphuber Thanks! I've rebased my PR to the latest develop branch, before there was no aiida.backends.tests.orm.data.test_data.TestData.

@simonpintarelli
Copy link
Contributor Author

Travis is failing the last test, because conda cannot find upf_to_json (https://pypi.org/project/upf-to-json/).

Is it enough to publish the package on conda-forge?

@sphuber
Copy link
Contributor

sphuber commented Oct 14, 2019

Yes, as long as it is on conda-forge, it should work. Note that even after pushing it to conda-forge it may take some time for it to become discoverable. But usually one hour after uploading should be sufficient.

@simonpintarelli
Copy link
Contributor Author

@sphuber, thank you. PR is pending on conda-forge: conda-forge/staged-recipes#9862

Copy link
Contributor

@sphuber sphuber left a comment

Choose a reason for hiding this comment

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

@simonpintarelli thanks a lot, this is looking great now. Except for a minor typo, this is looking good to merge. Could you please address the final change and rebase?

@export_options
@decorators.with_dbenv()
def upf_export(**kwargs):
"""Export StructureData object to file."""
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"""Export StructureData object to file."""
"""Export `UpfData` object to file."""

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I've fixed it.

UPF format 1 and 2 are supported. SIRIUS requires pseudopotentials in json
format.
Export a upf to json using:

verdi data export upf
@sphuber sphuber dismissed yakutovicha’s stale review October 29, 2019 08:50

Requested hanges addressed

@sphuber sphuber merged commit 2e3a9a7 into aiidateam:develop Oct 29, 2019
@sphuber
Copy link
Contributor

sphuber commented Oct 29, 2019

Thanks a lot for the contribution @simonpintarelli

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.

None yet

5 participants