Skip to content

Conversation

pmrv
Copy link
Contributor

@pmrv pmrv commented Jul 19, 2023

No description provided.

@jan-janssen
Copy link
Member

center_coordinates_in_unit_cell is not an ASE compatible command:

    s.center_coordinates_in_unit_cell()
AttributeError: 'Atoms' object has no attribute 'center_coordinates_in_unit_cell'

But you can use the structuretoolkit.common.center_coordinates_in_unit_cell() function to achieve the same.

Comment on lines 10 to 11
from pyxtal import pyxtal as _pyxtal
from pyxtal.msg import Comp_CompatibilityError
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
from pyxtal import pyxtal as _pyxtal
from pyxtal.msg import Comp_CompatibilityError

ValueError: if `species` and `num_ions` are not of the same length
ValueError: if stoichiometry and symmetry group are incompatible and allow_exceptions==False or only one structure is requested
"""
if len(species) != len(num_ions):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if len(species) != len(num_ions):
from pyxtal import pyxtal as _pyxtal
from pyxtal.msg import Comp_CompatibilityError
if len(species) != len(num_ions):

@jan-janssen
Copy link
Member

One goal of the structuretoolkit package is to remain very stream-lined. The default installation via pypi only requires ase, matplotlib, numpy and spicy, with the last three being requirements of ase. This is enabled by moving the imports into the functions, even though this conflicts with the PEP8 recommendations. Furthermore the tests are skipped when the corresponding packages are not available, this enables the validation of the minimal installation.

pmrv and others added 4 commits July 19, 2023 17:00
Co-authored-by: Jan Janssen <jan-janssen@users.noreply.github.com>
Co-authored-by: Jan Janssen <jan-janssen@users.noreply.github.com>
@pmrv
Copy link
Contributor Author

pmrv commented Jul 19, 2023

[I merged (and rebased) some of your changes before the force push]

One goal of the structuretoolkit package is to remain very stream-lined. The default installation via pypi only requires ase, matplotlib, numpy and spicy, with the last three being requirements of ase. This is enabled by moving the imports into the functions, even though this conflicts with the PEP8 recommendations. Furthermore the tests are skipped when the corresponding packages are not available, this enables the validation of the minimal installation.

I see the goal, but what about this way? This only conditionally defines the function and raises a warning if pyxtal is missing, so that users see it early and cannot even attempt to call the missing function. No strong feeling either, also happy to implement your original solution if you want it consistent everywhere.

Does sound like we want ImportAlarm here, though.

@jan-janssen
Copy link
Member

======================================================================
ERROR: test_return_value (test_pyxtal.TestPyxtal)
pyxtal should either return Atoms or list of dict, depending on arguments
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/runner/work/structuretoolkit/structuretoolkit/tests/test_pyxtal.py", line 36, in test_return_value
    self.assertIsInstance(pyxtal(1, species=['Fe'], num_ions=[1]), Atoms,
TypeError: 'module' object is not callable

----------------------------------------------------------------------

@jan-janssen jan-janssen merged commit 73f9926 into main Jul 22, 2023
@jan-janssen jan-janssen deleted the pyxtal branch July 22, 2023 04:27
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