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

Bond completion (or saturation) #202

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

tfrederiksen
Copy link
Contributor

The proposal of a nanoribbon geometry (#201) with edge-saturated atoms opened the idea to have more generally a method to complete bonds in geometries by adding extra atoms. This branch strives to achieve this.

@codecov
Copy link

codecov bot commented Apr 3, 2020

Codecov Report

Merging #202 into master will decrease coverage by 0.13%.
The diff coverage is 6.25%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #202      +/-   ##
==========================================
- Coverage   85.23%   85.10%   -0.14%     
==========================================
  Files         121      121              
  Lines       19367    19398      +31     
==========================================
+ Hits        16507    16508       +1     
- Misses       2860     2890      +30     
Impacted Files Coverage Δ
sisl/geometry.py 85.37% <6.25%> (-1.55%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 07fa7aa...a5f5d11. Read the comment docs.

@zerothi
Copy link
Owner

zerothi commented May 7, 2020

Thanks for the MR! Sorry for the late return. However, your request spawned an idea I would like some feedback on. Once that is in, your saturation feature could be generalized based on atomic categories ;-) and thus work for any system, including bulk surfaces etc. ;)

@pfebrer
Copy link
Contributor

pfebrer commented Aug 12, 2020

Hey! Can I update this PR to use categories so that it can be merged? It would help me with something that I need to do and I'd rather do it with an implementation in sisl than with some script in my computer that I won't be able to find in the future :) (Also, I'm sure it can be very useful for other people)

@pfebrer
Copy link
Contributor

pfebrer commented Aug 14, 2020

Now that we are updating things in categories, I think for this (and maybe other cases) it could be very useful that categories had an apply method that will execute a function for each atom that is in the category. Otherwise, I believe categories don't add much in this case because you need to do 2 loops: one for finding atoms with less than X neighbours and another one for acting on those atoms (and you need to calculate the neighbours again).

Any ideas how this could be made efficient?

@zerothi
Copy link
Owner

zerothi commented Aug 17, 2020

Hey! Can I update this PR to use categories so that it can be merged? It would help me with something that I need to do and I'd rather do it with an implementation in sisl than with some script in my computer that I won't be able to find in the future :) (Also, I'm sure it can be very useful for other people)

Yes, please try out this :)

Now that we are updating things in categories, I think for this (and maybe other cases) it could be very useful that categories had an apply method that will execute a function for each atom that is in the category. Otherwise, I believe categories don't add much in this case because you need to do 2 loops: one for finding atoms with less than X neighbours and another one for acting on those atoms (and you need to calculate the neighbours again).

I think this should be carefully thought through. Problem is that if you start changing the geometry state while holding a list of indices etc, stuff may change unexpectedly.
For now, I think we should not go this route and just settle with double calculation of neighbours. FYI, I initially did categories by storing state (number of neigbours, etc.), but that turned out to be extremely ugly and require quite a bit of documentation for end-users to extend. Hence I quickly abandoned it since it would mean that categories should ship with lots of secondary data.

@zerothi
Copy link
Owner

zerothi commented Aug 17, 2020

@tfrederiksen I hope it is ok if Pol takes a shot at this? Otherwise, chip in ;)

@tfrederiksen
Copy link
Contributor Author

No problem! :)

@pfebrer
Copy link
Contributor

pfebrer commented Aug 17, 2020

Problem is that if you start changing the geometry state while holding a list of indices etc, stuff may change unexpectedly.

Why do you need to change the state of the geometry?

I was thinking something like:

def saturate(...):
    new_ats = []
    new_xyz = []

    def get_saturating_ats(...):
         ...calculate the new positions # this will only run for those atoms that fall in the defined category
         new_ats.append(...)
         new_xyz.append(...)

    AtomCategory(neighbours=2).apply(get_saturating_ats)

   return geom + Geometry(new_xyz, new_ats)

However it's hard to figure out what to do when you want to apply on a composite category, because you may need the information that comes from all the categories, and that's probably where you need this

I initially did categories by storing state

😕

@pfebrer
Copy link
Contributor

pfebrer commented Aug 17, 2020

No problem! :)

@tfrederiksen I need permission to push to the branch to be able to update the PR :)

@zerothi
Copy link
Owner

zerothi commented Aug 17, 2020

No problem! :)

@tfrederiksen I need permission to push to the branch to be able to update the PR :)

Create a new one :) Much easier, just write it superseeds this one in the PR

@pfebrer
Copy link
Contributor

pfebrer commented Aug 17, 2020

Ok!

@zerothi
Copy link
Owner

zerothi commented Aug 17, 2020

Problem is that if you start changing the geometry state while holding a list of indices etc, stuff may change unexpectedly.

Why do you need to change the state of the geometry?

I was thinking something like:

def saturate(...):
    new_ats = []
    new_xyz = []

    def get_saturating_ats(...):
         ...calculate the new positions # this will only run for those atoms that fall in the defined category
         new_ats.append(...)
         new_xyz.append(...)

    AtomCategory(neighbours=2).apply(get_saturating_ats)

   return geom + Geometry(new_xyz, new_ats)

However it's hard to figure out what to do when you want to apply on a composite category, because you may need the information that comes from all the categories, and that's probably where you need this

I initially did categories by storing state

I don't think it should be like this, probably it would be nice if the saturate method could adapt to different saturation atoms (based on the category).

Something like:

cA = AtomCategory(...) # first category
cB = AtomCategory(...) # second category
cC = AtomCategory(...) # third category
c = cA | cB | cC
saturate_atoms = dict(cA={"distance": 1.4, "atom": Atom[3]}, cB={"distance": 1.5, "atom": Atom[4]}, cC=callback_function)
saturate(geometry, atoms=c, saturate_atoms=saturate_atoms)

or something similar?
This was why my first response to Thomas was that it should be more general so it could be used for surfaces etc. I also don't know if saturate would be the more natural name. Perhaps it could be merged with the Geometry.add method since it adds stuff based on common things. saturate assumes "dangling" bonds which isn't necessarily the case.

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