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

Gamma surface & stacking fault tools #178

Merged
merged 80 commits into from
Nov 24, 2023

Conversation

thomas-rocke
Copy link
Contributor

Classes to generate stacking fault & gamma surface (generalised stacking fault) images, plus utils for managing relaxation, evaluation, & plotting.

Class for generating gamma surface/generalised stacking fault images & plots
Shell class for convenient stacking fault images/measurements
@pgrigorev
Copy link
Contributor

Hi @thomas-rocke, this looks like a very good idea and nice work. Gamma surface and stacking faults are strongly related to dislocation properties and this is why I had an implementation of a gamma_line function. The idea was that you can pass the unit_cell from the dislocation class to this function and use it to provide related gamma line and stacking fault energy. I see that you added a deprecation to the function in dislocation module. I guess your implementation is more general and I will be happy to use it in the future. It would be great if we could discuss how I can use your new implementation instead of one in the dislocation module. Also we should think about a tutorial example ideally closely related to the dislocation examples. For example how to use this module to get stacking faults for 1/2<111>(110) dislocation and/or other dislocations in BCC. Let me know what you think?

@thomas-rocke
Copy link
Contributor Author

Hi @pgrigorev , I see your point about dislocations and gamma surfaces being very closely related - there's definitely an argument in trying to modify my solution to more directly coexist with/around the dislocation tools, and I have a few ideas on how we could achieve this. I can also make a reasonable tutorial on utilising the code, once we've settled on the exact final solution. I'm happy to meet to discuss this at some point, maybe also including @jameskermode ?

@pgrigorev
Copy link
Contributor

Sure I will be happy to discuss together. Also to understand better what you did. For the tutoral I was thinking to add a section to plasticity documentation about calculation of stacking fault energies relevant for different dislocations. You would just to need to create a Jupiter notebook with and it will be automatically rendered for the documentation.

@thomas-rocke thomas-rocke reopened this Nov 8, 2023
@thomas-rocke
Copy link
Contributor Author

Successfully added the ability to generate stacking faults from dissociated dislocations (e.g. StackingFault(alat, DiamondGlideScrew, symbol="Si")) and get correct structures from CNA. Docs are a WIP, and I think I need more testing of the results before this is ready for release.

@thomas-rocke
Copy link
Contributor Author

@jameskermode @pgrigorev New docs are done, and I think the features are also finished (I did relax the dislocation constraint so any CubicCrystalDislocation is fine). If I'm being critical, I should maybe try to have a few more unit tests, but I'm unsure what the best practice should be - I don't think it's useful to have a bunch of individual tests for very basic things, if they are just going to get picked up by the energy fits anyway.

Assuming there's nothing massively wrong with the docs, we should be good to try to merge this on Weds.

@thomas-rocke thomas-rocke marked this pull request as ready for review November 20, 2023 11:02
@jameskermode jameskermode changed the title WIP: Gamma surface & stacking fault tools Gamma surface & stacking fault tools Nov 22, 2023
Copy link
Member

@jameskermode jameskermode left a comment

Choose a reason for hiding this comment

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

Approved subject to remaining small changes and @pgrigorev being happy with the docs

docs/applications/plasticity.rst Outdated Show resolved Hide resolved
docs/applications/plasticity.rst Outdated Show resolved Hide resolved
matscipy/dislocation.py Outdated Show resolved Hide resolved
matscipy/gamma_surface.py Outdated Show resolved Hide resolved
matscipy/gamma_surface.py Outdated Show resolved Hide resolved
matscipy/gamma_surface.py Outdated Show resolved Hide resolved
matscipy/gamma_surface.py Outdated Show resolved Hide resolved
matscipy/gamma_surface.py Outdated Show resolved Hide resolved
Copy link
Contributor

@pgrigorev pgrigorev left a comment

Choose a reason for hiding this comment

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

There were still a few complaints left. I does not have to be perfect, but unused variables can be very confusing. Once it is fixed and everything works, I am happy with this to be merged.

Great work. Thanks a lot!

matscipy/gamma_surface.py Outdated Show resolved Hide resolved
matscipy/gamma_surface.py Outdated Show resolved Hide resolved
matscipy/gamma_surface.py Show resolved Hide resolved
matscipy/gamma_surface.py Show resolved Hide resolved
matscipy/gamma_surface.py Outdated Show resolved Hide resolved
@thomas-rocke
Copy link
Contributor Author

@jameskermode this should be finished and ready to merge. Most of the new changes are just small QOL fixes.

One new feature of note has been added. @pgrigorev had spotted that gamma_surface.ipynb was silently failing in the CI Docs build due to timeout. He extended the timeout or that notebook, and I've added the nb_execution_raise_on_error=True arg to the sphinx conf.py, which should make the CI fail if there are any future issues with notebooks. It definitely works locally with a notebook I knew would fail.

@pgrigorev
Copy link
Contributor

@jameskermode this should be finished and ready to merge. Most of the new changes are just small QOL fixes.

One new feature of note has been added. @pgrigorev had spotted that gamma_surface.ipynb was silently failing in the CI Docs build due to timeout. He extended the timeout or that notebook, and I've added the nb_execution_raise_on_error=True arg to the sphinx conf.py, which should make the CI fail if there are any future issues with notebooks. It definitely works locally with a notebook I knew would fail.

I did not know about this flag, this is great that you found it. I think it is in shape to be merged. Fantastic work!

@jameskermode jameskermode merged commit 773eb12 into libAtoms:master Nov 24, 2023
15 checks passed
@jameskermode
Copy link
Member

thanks @thomas-rocke and also to @pgrigorev for reviewing!

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.

3 participants