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

WIP: update api for python module, drop python2 #777

Merged
merged 13 commits into from
Nov 11, 2022
Merged

Conversation

simonpintarelli
Copy link
Collaborator

@simonpintarelli simonpintarelli commented Nov 4, 2022

  • drop python2
  • update to new SIRIUS API for most important parts
  • cleanup / split py_sirius.cpp into multiple cpp files
  • @toxa81 ci/cd pipeline for python module

@simonpintarelli simonpintarelli marked this pull request as draft November 4, 2022 10:45
@@ -95,7 +95,7 @@ class Sirius(CMakePackage, CudaPackage):
depends_on('pkgconfig', type='build')

# Python module
depends_on('python', when='+python', type=('build', 'run'))
depends_on('python', when='+python@3.6:', type=('build', 'run'))
Copy link
Collaborator

Choose a reason for hiding this comment

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

IMO we should drop this file as well and submit changes directly to the spack package of sirius

@toxa81
Copy link
Collaborator

toxa81 commented Nov 11, 2022

The vector3d intrface is not working properly. This works but is awkward:
https://github.com/electronic-structure/SIRIUS/blob/update/python-module/examples/python/Si_dft.py#L29

This should be the intrface (https://github.com/electronic-structure/SIRIUS/blob/update/python-module/examples/python/Si_dft.py#L30), but it gives the error:

TypeError: add_atom(): incompatible function arguments. The following argument types are supported:
    1. (self: sirius.py_sirius.Unit_cell, arg0: str, arg1: geometry3d::vector3d<double>) -> None

@simonpintarelli
Copy link
Collaborator Author

The vector3d intrface is not working properly. This works but is awkward: https://github.com/electronic-structure/SIRIUS/blob/update/python-module/examples/python/Si_dft.py#L29

This should be the intrface (https://github.com/electronic-structure/SIRIUS/blob/update/python-module/examples/python/Si_dft.py#L30), but it gives the error:

TypeError: add_atom(): incompatible function arguments. The following argument types are supported:
    1. (self: sirius.py_sirius.Unit_cell, arg0: str, arg1: geometry3d::vector3d<double>) -> None

I'll have a look.

@simonpintarelli
Copy link
Collaborator Author

@toxa81 Now both variants Unit_cell.add_atom(label, [list_like]) and Unit_cell.ad_atom(label, sirius.vector3d). The former also does check the length, it will accept only python list / numpy arrays with 3 entries (pybind11 does that).

@toxa81
Copy link
Collaborator

toxa81 commented Nov 11, 2022

This looks good to me now. Unless @simonpintarelli you really want to work on cleanup / split py_sirius.cpp into multiple cpp files, it is good to go for meging. The Python intrface is restored to a minimum that can run DFT ground state.

@simonpintarelli
Copy link
Collaborator Author

@toxa81 Let's merge it. I'll work on it later.

@toxa81 toxa81 marked this pull request as ready for review November 11, 2022 22:59
@toxa81 toxa81 merged commit caa2d2a into develop Nov 11, 2022
@toxa81 toxa81 deleted the update/python-module branch November 19, 2022 00:36
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