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

Check "cell" input signature in python interface #412

Closed
atztogo opened this issue Jan 30, 2024 · 4 comments · Fixed by #420
Closed

Check "cell" input signature in python interface #412

atztogo opened this issue Jan 30, 2024 · 4 comments · Fixed by #420

Comments

@atztogo
Copy link
Collaborator

atztogo commented Jan 30, 2024

As raised in #411, this should be fixed by something like not isinstance(cell, tuple) and not len(cell) in (3, 4) as @sphuber suggested.

cell is defined at https://spglib.readthedocs.io/en/latest/python-interface.html#crystal-structure-cell. I think cell can be given by list or something like that. How should we do? Do you have any idea @lan496, @LecrisUT?

@lan496
Copy link
Member

lan496 commented Jan 31, 2024

I also agree to raise an exception when cell has an invalid type. This can be harmlessly done by emitting an exception from spglib._check:

spglib/python/spglib/spglib.py

Lines 1999 to 2013 in 63a7a4b

def _check(lattice, positions, numbers, magmoms):
if lattice.shape != (3, 3):
return False
if positions.ndim != 2:
return False
if positions.shape[1] != 3:
return False
if numbers.ndim != 1:
return False
if len(numbers) != positions.shape[0]:
return False
if magmoms is not None:
if len(magmoms) != len(numbers):
return False
return True

The only thing to be discussed will be a name for the exception. For example CellError?

@LecrisUT
Copy link
Collaborator

LecrisUT commented Feb 1, 2024

Use standard TypeError and such. Probably individual for each one. I don't think the user would have much use for a CellError there since they are already in charge of the type they pass.

Custom exception would be useful for convergence or internal errors.

@lan496
Copy link
Member

lan496 commented Feb 1, 2024

Indeed. TypeError will be appropriate.

@lan496
Copy link
Member

lan496 commented Feb 2, 2024

I'll work on it.

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 a pull request may close this issue.

3 participants