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

Pyscal solid liquid #414

Merged
merged 7 commits into from
Nov 9, 2021
Merged

Pyscal solid liquid #414

merged 7 commits into from
Nov 9, 2021

Conversation

Leimeroth
Copy link
Member

Adds pyscals ability to find solid and liquid atoms in a structure.
Also adds a get_system convenience function to have easier access to pyscal functionality that is not directly accessible and replaces repetitive code with the get_system function.

sys.find_neighbors(method="voronoi")
atoms = sys.atoms
return np.array([atom.volume for atom in atoms])

def get_system(atoms):
Copy link
Contributor

Choose a reason for hiding this comment

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

I think for consistency we would want to call that pyiron_to_pyscal, similar to pyiron_to_ase and pyiron_to_pymatgen.

Copy link
Member Author

Choose a reason for hiding this comment

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

renamed it. I also moved it next to the to_ase, to_pymatgen and to_ovito methods

)
return sys

def analyse_find_solids(atoms, neighbor_method="cutoff",
Copy link
Contributor

Choose a reason for hiding this comment

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

This returns the number of atoms that are inside a solid phase? If so it'd cool to also return an index array, that shows which are solid and which are liquid, maybe behind a switch, like the pyscal_adaptive_cna method.

Copy link
Member Author

Choose a reason for hiding this comment

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

This only returns the number by default, but when using return_system=True it returns the complete pyscal system object, which allows to get the indices. I thought this is the most simple in the default case and the most flexible approach when using the return_system option.
Alternatively I could change the default to return an array with the indices of all atoms that are identified as solid, so that the len(returned_array) gives the number of solids I guess. But I do not know if the order of the atoms stays the same when converting between pyiron ase and pyscal system, so I am not sure if indices will always be correct

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I had simply missed that while skimming. Since anything might happen while changing from pyiron to pyscal returning the full system is ok, I suppose.

return_sys=False,
):
"""
Get the number of solids or the corresponding pyscal system.
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo?

Copy link
Member Author

Choose a reason for hiding this comment

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

what do you mean?

Copy link
Contributor

Choose a reason for hiding this comment

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

Since I misunderstood the method, I thought or should be of or some such... I promise to read more carefully next time. ;)

@Leimeroth
Copy link
Member Author

I think the tests are failing due to #409, since the error messages seem unrelated to my changes. I will wait till this is sorted out and than merge this when tests run, if you don't see any other problems @pmrv.

@pmrv
Copy link
Contributor

pmrv commented Nov 9, 2021

Yep, once the h5io is sorted out, go ahead and merge.

@pmrv pmrv self-requested a review November 9, 2021 13:24
@niklassiemer
Copy link
Member

I fixed the h5io version (for now) in #410, thus, try to rerun the tests :)

@coveralls
Copy link

Pull Request Test Coverage Report for Build 1439141654

  • 14 of 24 (58.33%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.04%) to 69.723%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pyiron_atomistics/atomistics/structure/analyse.py 1 2 50.0%
pyiron_atomistics/atomistics/structure/atoms.py 2 3 66.67%
pyiron_atomistics/atomistics/structure/pyscal.py 11 19 57.89%
Totals Coverage Status
Change from base Build 1413541866: -0.04%
Covered Lines: 11408
Relevant Lines: 16362

💛 - Coveralls

@liamhuber
Copy link
Member

Cool! We are really getting a lot of pyscal mapped functions -- what about doing a refactor (after this PR, not part of it) so that we nest these bindings, i.e. structure.analyse.pyscal.find_solids instead of structure.analyse.pyscal_find_solids?

@Leimeroth
Copy link
Member Author

Cool! We are really getting a lot of pyscal mapped functions -- what about doing a refactor (after this PR, not part of it) so that we nest these bindings, i.e. structure.analyse.pyscal.find_solids instead of structure.analyse.pyscal_find_solids?

I always tried to type exactly that.

@Leimeroth Leimeroth merged commit d0e6af4 into master Nov 9, 2021
@delete-merged-branch delete-merged-branch bot deleted the pyscal_solid_liquid branch November 9, 2021 14:41
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.

5 participants