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

Use Structuretoolkit #994

Merged
merged 12 commits into from
Mar 30, 2023
Merged

Use Structuretoolkit #994

merged 12 commits into from
Mar 30, 2023

Conversation

jan-janssen
Copy link
Member

@jan-janssen jan-janssen requested a review from samwaseda March 28, 2023 17:19
@jan-janssen
Copy link
Member Author

This pull request is part of the greater effort to separate pyiron into more stand-alone packages. This change gives users the chance to benefit from pyiron without having to switch their whole workflow to pyiron. In this specific case, new users can benefit from our neighbor analysis and general analysis for atomistic structures, by combining the structuretoolkit with ASE atomistic structures. The hope is that when users enjoy the software developed by the pyiron developers then they are hopefully also interested to use the pyiron project as a whole. Now this pull request removes the code which is now included in the structuretoolkit and just imports the corresponding functionality.

@jan-janssen jan-janssen merged commit 3183fe6 into main Mar 30, 2023
@delete-merged-branch delete-merged-branch bot deleted the structuretoolkit branch March 30, 2023 20:49
@samwaseda
Copy link
Member

Sorry I missed this review request (I’m also on vacation right now). But this import statement currently breaks my notebooks. Is it my problem (which obviously I can fix by cloning the repository), or is it more a major problem?

@jan-janssen
Copy link
Member Author

You can install structuretoolkit via conda that should fix the import issue, at least the tests all pass.

@samwaseda
Copy link
Member

It doesn’t sound like a solution for pyiron

@jan-janssen
Copy link
Member Author

It doesn’t sound like a solution for pyiron

It is part of the dependencies so for regular users it is installed automatically. Only developers have to manually install the dependencies.

@samwaseda
Copy link
Member

But I still don’t understand why it’s not automatically installed on my side when I install pyiron. It’s not released yet as an official pyiron version?

@jan-janssen
Copy link
Member Author

No, the changes were merged three days ago and there was no release of a new official pyiron version since then. It is going to be included with the next official release.

@samwaseda
Copy link
Member

It actually makes me feel that this splitting would create only discrepancies. Is there a particular reason that it should be a separate package?

@jan-janssen
Copy link
Member Author

It actually makes me feel that this splitting would create only discrepancies. Is there a particular reason that it should be a separate package?

The original discussion was in https://github.com/pyiron/team/discussions/162 . The split is essential part of the strategy to make parts of pyiron accessible to a larger audience without forcing them to us pyiron as a whole. The structuretoolkit is fully compatible to the ASE atoms class which also helps us to focus on the migration away from the pyiron atoms class towards the ASE atoms class. In the mid-term the goal is to have the job management on top of the structuretoolkit and individual wrappers like pyiron_lammps, similar to the way how dask is used on top of python, rather than requiring each python class to be derived from a dask class.

@samwaseda
Copy link
Member

Ok that’s more or less the argument that I had expected. I don’t oppose it, but I also have the feeling that we don’t have a clear picture of how pyiron_atomistics should be split, or it hasn’t really been talked through. And at the pyiron meeting you presented pyiron_lammps and structuretoolkit as experimental features, so I’m now a bit confused that it is now included in the official pyiron version.

But ok anyway let’s keep it like this.

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