Function to build heteroribbons #421

pfebrer · 2022-01-12T18:14:58Z

Closes #420

I still need confirmation from @tfrederiksen that there's nothing super wrong with the way it can be used :)

Is the code structure/arguments fine?

pfebrer · 2022-01-12T18:18:21Z

Also if you prefer other names for the functions I don't mind, Thomas used composite, I don't know which one is better.

codecov · 2022-01-12T18:28:27Z

Codecov Report

Merging #421 (eefa952) into main (fc1da2c) will decrease coverage by 0.01%.
The diff coverage is 94.72%.

@@            Coverage Diff             @@
##             main     #421      +/-   ##
==========================================
- Coverage   87.34%   87.34%   -0.01%     
==========================================
  Files         373      374       +1     
  Lines       47705    47959     +254     
==========================================
+ Hits        41669    41889     +220     
- Misses       6036     6070      +34

Impacted Files	Coverage Δ
src/sisl/__init__.py	`97.56% <ø> (ø)`
src/sisl/_environ.py	`86.84% <ø> (ø)`
src/sisl/_help.py	`69.82% <0.00%> (ø)`
src/sisl/_typing.py	`0.00% <0.00%> (ø)`
src/sisl/geom/category/base.py	`92.85% <ø> (ø)`
src/sisl/io/_help.py	`82.14% <ø> (ø)`
src/sisl/io/bigdft/sile.py	`100.00% <ø> (ø)`
src/sisl/io/gulp/sile.py	`100.00% <ø> (ø)`
src/sisl/io/orca/sile.py	`100.00% <ø> (ø)`
src/sisl/io/scaleup/sile.py	`100.00% <ø> (ø)`
... and 252 more

... and 38 files with indirect coverage changes

zerothi

Great job! Minor changes here and there. @tfrederiksen the name of the function, what do you think?

sisl/geom/nanoribbon.py

tfrederiksen · 2022-01-13T10:14:55Z

Great work @pfebrer ! Overall it looks great to me. However, I am not fully convinced about the use of imaginary numbers to specify "open" structures. I think it follows naturally that each consequtive "column" in the structure needs to be of alternating type. Hence, for a given input of widths and repetitions are only two possible structures that can be created. This could be one flag to the function, say invert=True/False. What do you think?

tfrederiksen · 2022-01-13T10:33:48Z

Great job! Minor changes here and there. @tfrederiksen the name of the function, what do you think?

I think it is good (and relatively short) as it is. In the literature I see mostly the formulation "graphene nanoribbon heterojunctions" for these structures. Other possibilities could therefore be the pair nanoribbon_heterojunction and gnr_heterojunction? (Or composite_nanoribbon and composite_gnr?)

Could the method eventually be extended to 2D nanomeshes?

zerothi · 2022-01-13T10:45:51Z

I think it is good (and relatively short) as it is. In the literature I see mostly the formulation "graphene nanoribbon heterojunctions" for these structures. Other possibilities could therefore be the pair nanoribbon_heterojunction and gnr_heterojunction? (Or composite_nanoribbon and composite_gnr?)

I like nanoribbon_hetero, kind of feel the "junction" is just too long. ;)

Hmm.. On the other hand explicitness is good... hmmm. Since hetero is already in the litterature, I like that name.

pfebrer · 2022-01-13T11:29:46Z

I think it follows naturally that each consequtive "column" in the structure needs to be of alternating type. Hence, for a given input of widths and repetitions are only two possible structures that can be created. This could be one flag to the function, say invert=True/False.

It follows naturally in most cases, but there are two specific cases where it does not follow naturally:

At the beginning of the unit cell: The configuration of the initial string forces the creation of completely different ribbons. One can't be related to the other just by a simple invert operation. As you noted, you probably can always find a string of atoms where you can start with the "sisl provided" configuration and then shift, but I don't believe that this is a good solution :(
When an even section comes in and the previous section was odd: This case is fully defined if you specify the initial atom/final atom as in your approach of course, but not in this case where the ribbons are automatically aligned and 0 means "aligned". There are two valid ways to "perfectly align" an odd ribbon with an even ribbon:
- If the bottom edge of the even ribbon is open ("closed" configuration), they are aligned at the bottom:
```
graphene_heteroribbon([(7, 1), (8, 1)])
```
  In the case where the previous section is closed, a shift is then allowed, but only upwards, as a downward shift would leave the last atom of the even section dangling:
```
graphene_heteroribbon([(7, 2), (8, 1, 1)])
```
- If the top edge of the even ribbon is open ("open" configuration), they are aligned at the top:
```
graphene_heteroribbon([(7, 1), (8, 1j)])
```
  And then in the case when the previous section is closed, only a downwards shift is allowed.
If there is only one such choice, the obtained final structures are related by a mirror symmetry, certainly, as you note. But if there is more than one such choice, then it isn't the case anymore. Therefore, you need to have the ability to choose.

That's why I only allow imaginary numbers in these two cases. We could provide an extra field to specify this change instead of using this notation, but as I told Nick this has the pitfall that you need to specify all the parameters for the section, because sections are passed as tuples. Whatever you prefer.

pfebrer · 2022-01-13T11:31:41Z

I like nanoribbon_hetero, kind of feel the "junction" is just too long. ;)

But then the graphene version of it would be graphene_nanoribbon_hetero ? 😅

zerothi · 2022-01-13T11:36:18Z

That's why I only allow imaginary numbers in these two cases. We could provide an extra field to specify this change instead of using this notation, but as I told Nick this has the pitfall that you need to specify all the parameters for the section, because sections are passed as tuples. Whatever you prefer.

But that is the same as the other arguments in the Section tuple, no?

I like nanoribbon_hetero, kind of feel the "junction" is just too long. ;)

But then the graphene version of it would be graphene_nanoribbon_hetero ? sweat_smile

Agreed... Hmm....
I also like your original one.. I don't know ;) !

pfebrer · 2022-01-13T11:41:00Z

But that is the same as the other arguments in the Section tuple, no?

Yes, it's just that the more parameters you add, the worse it gets :) Specially if parameters are not easily ordered by importance. E.g. we can all agree that length will be very used so it makes sense that it is the first optional argument.

pfebrer · 2022-01-13T11:44:57Z

Also, for the name: It is not really a heterojunction but potentially several heterojunctions, so probably calling it heterojunction would not picture what the function does exactly.

zerothi · 2022-01-13T11:45:19Z

But that is the same as the other arguments in the Section tuple, no?

Yes, it's just that the more parameters you add, the worse it gets :) Specially if parameters are not easily ordered by importance. E.g. we can all agree that length will be very used so it makes sense that it is the first optional argument.

True... Perhaps you should add the possibility of accepting dict as sections.

     sections = [Heteroribbon_section(*section) for section in sections]
# to
def conv(s):
    if isinstance(s):
        return Heteroribbon_section(**s)
    return Heteroribbon_section(*s)
sections = [conv(section) for section in sections]

pfebrer · 2022-01-13T11:47:26Z

Well in that case you might as well build the section object yourself instead of passing a dict :)

zerothi · 2022-01-13T11:49:07Z

Well in that case you might as well build the section yourself instead of passing a dict :)

In any case I think it might be useful for some.

pfebrer · 2022-01-13T11:50:07Z

However would you then introduce the open parameter? That forces the user to replace a list with a dict just to change the configuration.

pfebrer · 2022-01-13T11:51:19Z

The imaginary number seems natural to me. 2j is just 2 shifted by a phase :)

zerothi · 2022-01-13T11:52:39Z

I don't know about this, @tfrederiksen what do you say?

pfebrer · 2022-01-13T12:03:24Z

Another option could be using negative numbers (-2 instead of 2j)

tfrederiksen · 2022-01-13T12:07:15Z

At the beginning of the unit cell: The configuration of the initial string forces the creation of completely different ribbons. One can't be related to the other just by a simple invert operation.

I think I didn't express my idea correctly. It is not to apply an invert operation after creating the structure, but to let the user select the "phase" of the first column (open/closed). I fully agree that this choice will create different structures. But the consecutive columns must respect phase alternation, no?. Indeed the meaning of shift would have to be redefined (no longer just multiples of the lattice vector in y).

My point is just that from the user perspective it may be simpler if one does not have to keep track of the phase, but just specify opening type and then widths/reps/shifts.

Edit: Additionally, in my opinion it is not very clear from the user perspective if a given "column" should be denoted "closed" (real) or "open" (imaginary). It seems ambiguous to me.

pfebrer · 2022-01-13T12:11:41Z

Oooh ok I understand now, so

graphene_nanoribbon([(7, 2), (11, 2)], open_start=True) # or invert=True

would be the old

graphene_nanoribbon([(7, 2j), (11, 2)])

?

I like it :) But then the second case still remains.

tfrederiksen · 2022-01-13T12:15:43Z

Oooh ok I understand now, so

graphene_nanoribbon([(7, 2), (11, 2)], open_start=True) # or invert=True

would be the old

graphene_nanoribbon([(7, 2j), (11, 2)])

?

Exactly!

I like it :) But then the second case still remains.

What do you mean by second case? The meaning of shift would then have to refer to half the lattice vector in y.

pfebrer · 2022-01-13T12:18:58Z

Additionally, in my opinion it is not very clear from the user perspective if a given "column" should be denoted "closed" (real) or "open" (imaginary). It seems ambiguous to me.

For odd columns I think it's very well defined. Why not? You have to consider if you are looking at it from the left or the right, but that seems fine to me. I mean I do not call the column "open" or close, the same column can be an open or closed border. For even columns it is arbitrary I agree, but once you establish which is which it isn't ambiguous either.

tfrederiksen · 2022-01-13T12:26:19Z

Additionally, in my opinion it is not very clear from the user perspective if a given "column" should be denoted "closed" (real) or "open" (imaginary). It seems ambiguous to me.

For odd columns I think it's very well defined. Why not? You have to consider if you are looking at it from the left or the right, but that seems fine to me. I mean I do not call the column "open" or close, the same column can be an open or closed border. For even columns it is arbitrary I agree, but once you establish which is which it isn't ambiguous either.

My point is just that understanding all of this would not be required by the user. One could just choose one of two possible "openings" and then build anything from there.

pfebrer · 2022-01-13T12:32:00Z

What do you mean by second case? The meaning of shift would then have to refer to half the lattice vector in y.

Hmm ok, then you'd have to, for example always align at the bottom and then shift. I get your point. But I don't think that makes it more intuitive necessarily. That might make it harder to build valid ribbons because you have to know the allowed shift values. The main point of building this function for me was that I didn't have to think about which shifts are valid and which are not. I like the idea of just going by:

0: aligned
-1: first possible downward shift (if available)
1: first possible upwards shift (if available)

If the user needs to specify exactly where to place the ribbon (your proposal) I believe they have to think even more about phases, since otherwise they won't generate valid ribbons.

tfrederiksen · 2022-01-13T12:35:44Z

What do you mean by second case? The meaning of shift would then have to refer to half the lattice vector in y.

Hmm ok, then you'd have to, for example always align at the bottom and then shift. I get your point. But I don't think that makes it more intuitive necessarily. That might make it harder to build valid ribbons because you have to know the allowed shift values. The main point of building this function for me was that I didn't have to think about which shifts are valid and which are not. I like the idea of just going by:
* `0`: aligned

* `-1`: first possible downward shift (if available)

* `1`: first possible upwards shift (if available)
If the user needs to specify exactly where to place the ribbon (your proposal) I believe they have to think even more about phases, since otherwise they won't generate valid ribbons.

All integer shifts should be valid (in units of half the lattice vector). But clearly the function needs to pick the only meaningful phase for that column not to mess up the bonding pattern.

pfebrer · 2022-01-13T12:36:59Z

No, they are not, because some shifts leave dangling bonds at the edges of the ribbon

zerothi · 2022-01-28T12:52:10Z

After discussing with @pfebrer we have come up with some other ideas that @pfebrer will start exploring. :)

I will commence the 0.12 release and then this may enter in a minor release at a later point.

zerothi · 2023-01-09T08:21:21Z

@pfebrer we had a meeting and you were doing some explorations, but now I can't recall those explorations? What is the status here?

pfebrer · 2023-01-09T13:03:10Z

We thought about having some way to merge geometries in a smart way by adding two classes: Section and Junction,
where a junction would know and control how to merge the two geometries, doing something like this:

full_geom = Section() + Junction() + Section() + ...

Then we thought that we shouldn't limit it to Junctions and in general this could be Actions, being Section also something more general like GeometrySpec or whatever. I think we thought about this in particular to create defects.

full_geom = MetalSurface("Au", "111") + DefectAction() + Junction(adapt_to_lattice=0) + MetalSurface("Cu", "111")

for example would create an Au surface, then create a defect on it, and then join it with a Cu surface that would use the same lattice as gold.

This was a nice idea but I couldn't find a way to make it general and at the same time not much more difficult to use than the current approach for graphene nanoribbons which is just based on passing a list of tuples.

Now this is stalled, and for now I did not have plans to work on it 😅

pfebrer · 2023-01-09T13:05:37Z

The advantage of this approach was that there would be a general API for creating composite structures, instead of each method implementing one.

zerothi · 2023-01-09T19:49:09Z

Great, thanks for the reminder.

Ok, so what about I run it through again, and then we just merge this? Then we can always think about the next thing.

After having read your recap of our conversation, perhaps the most ideal thing would be to fix Geometry.add/append to accept a class that fixes the merging instead of relying on axis fixtures etc.

pfebrer · 2023-01-09T20:24:36Z

Ok, so what about I run it through again, and then we just merge this? Then we can always think about the next thing.

What do you mean by "run it through"?

After having read your recap of our conversation, perhaps the most ideal thing would be to fix Geometry.add/append to accept a class that fixes the merging instead of relying on axis fixtures etc.

Yes but that would require the Geometry itself to have some knowledge of which type of geometry it is. Otherwise this merging class will have no idea what to do with it.

zerothi · 2023-01-09T20:43:11Z

Ok, so what about I run it through again, and then we just merge this? Then we can always think about the next thing.

What do you mean by "run it through"?

I'll check this PR again.

After having read your recap of our conversation, perhaps the most ideal thing would be to fix Geometry.add/append to accept a class that fixes the merging instead of relying on axis fixtures etc.

Yes but that would require the Geometry itself to have some knowledge of which type of geometry it is. Otherwise this merging class will have no idea what to do with it.

True, hmm...

zerothi · 2023-07-10T09:18:31Z

I am thinking this could be helpful. So I think we should merge it. :)

zerothi · 2023-07-10T09:18:38Z

Any comments @tfrederiksen @pfebrer

src/sisl/geom/tests/test_geom.py

+
+
+def test_graphene_heteroribbon():
+    a = graphene_heteroribbon([(7, 2), (9, 2)])


src/sisl/geom/nanoribbon.py

+                aligned_match = last_bot_edge_open == this_bot_edge_open
+
+            # Shift the incoming section if the vertical shift makes them not match.
+            if aligned_match == (shift % 2 == 1):


pfebrer · 2023-07-10T11:09:48Z

The only pitfall I see is that this introduces some kind of "framework" for composite structures, which might be a pain in the future if you want to go in another direction. But if you think it's fine, then OK 👍

zerothi · 2023-07-10T11:12:15Z

Agreed, but if you think this functionality is valuable, I think we should merge it. Then we can always move things to a deprecated folder and introduce the new method over time.... I think it has a long time perspective to get a better overall working scheme.

pfebrer · 2023-07-10T11:15:38Z

Ok, is it properly documented (can't explore it now I'm on the metro)? If it is then for me it is ok to merge it. Otherwise I can try to document it this afternoon or tomorrow.

zerothi · 2023-07-10T11:18:53Z

I'll have a go and ask for another go by you. I think the section classes could be hosted in the function as a property

def heteroribbon(...):


heteroribbon.section = _heteroribbon_section

or similar.

src/sisl/geom/tests/test_geom.py

@@ -1,10 +1,14 @@
 # This Source Code Form is subject to the terms of the Mozilla Public
 # License, v. 2.0. If a copy of the MPL was not distributed with this
 # file, You can obtain one at https://mozilla.org/MPL/2.0/.
+import itertools
+import math as m


src/sisl/geom/_composite.py

+
+    @abstractmethod
+    def build_section(self, geometry):
+        ...


src/sisl/geom/_composite.py

+
+    @abstractmethod
+    def add_section(self, geometry, geometry_addition):
+        ...


Signed-off-by: Nick Papior <[email protected]>

Now they are exposed Signed-off-by: Nick Papior <[email protected]>

Signed-off-by: Nick Papior <[email protected]>

zerothi · 2023-07-16T20:01:31Z

@pfebrer feel free to have a look now :)

pfebrer · 2023-07-17T10:49:16Z

Yeah I think it's fine! Thank you for the polishing :)

pfebrer changed the title ~~Function to build heteroribbonds~~ Function to build heteroribbons Jan 12, 2022

pfebrer force-pushed the heteroribbons branch from 11a5374 to de20144 Compare January 12, 2022 18:22

zerothi requested changes Jan 12, 2022

View reviewed changes

zerothi reviewed Jan 12, 2022

View reviewed changes

sisl/geom/nanoribbon.py Outdated Show resolved Hide resolved

zerothi reviewed Jan 12, 2022

View reviewed changes

sisl/geom/nanoribbon.py Outdated Show resolved Hide resolved

zerothi force-pushed the heteroribbons branch from 56b09d6 to b7e8137 Compare July 10, 2023 09:18

github-advanced-security bot found potential problems Jul 10, 2023

View reviewed changes

zerothi force-pushed the heteroribbons branch from 136e7eb to fb6a47e Compare July 10, 2023 20:23

github-advanced-security bot found potential problems Jul 10, 2023

View reviewed changes

pfebrer and others added 7 commits July 15, 2023 21:01

new paradigm

0dafb02

bug: less buggy way of shifting the cell

201c953

bug: corrected some bugs on valid shifts

331d3e4

fixed tests after rebasing

076e18d

Signed-off-by: Nick Papior <[email protected]>

fixed tests and docs

628cee4

Signed-off-by: Nick Papior <[email protected]>

rebase isort fixes

22c51e1

Signed-off-by: Nick Papior <[email protected]>

renamed section class names

1915527

Now they are exposed Signed-off-by: Nick Papior <[email protected]>

zerothi force-pushed the heteroribbons branch from b6baeb7 to 1915527 Compare July 15, 2023 19:01

added CompositeGeometrySection to docs

eefa952

Signed-off-by: Nick Papior <[email protected]>

pfebrer mentioned this pull request Jul 17, 2023

Passivate edges in carbon nanostructures #600

Open

zerothi merged commit 2754537 into zerothi:main Jul 17, 2023



		def test_graphene_heteroribbon():
		a = graphene_heteroribbon([(7, 2), (9, 2)])

Function to build heteroribbons #421

Function to build heteroribbons #421

Conversation

pfebrer commented Jan 12, 2022

pfebrer commented Jan 12, 2022

codecov bot commented Jan 12, 2022 • edited Loading

Codecov Report

zerothi left a comment

Choose a reason for hiding this comment

tfrederiksen commented Jan 13, 2022

tfrederiksen commented Jan 13, 2022

zerothi commented Jan 13, 2022

pfebrer commented Jan 13, 2022

pfebrer commented Jan 13, 2022

zerothi commented Jan 13, 2022

pfebrer commented Jan 13, 2022

pfebrer commented Jan 13, 2022

zerothi commented Jan 13, 2022

pfebrer commented Jan 13, 2022 • edited Loading

zerothi commented Jan 13, 2022

pfebrer commented Jan 13, 2022

pfebrer commented Jan 13, 2022

zerothi commented Jan 13, 2022

pfebrer commented Jan 13, 2022

tfrederiksen commented Jan 13, 2022 • edited Loading

pfebrer commented Jan 13, 2022 • edited Loading

tfrederiksen commented Jan 13, 2022

pfebrer commented Jan 13, 2022

tfrederiksen commented Jan 13, 2022

pfebrer commented Jan 13, 2022

tfrederiksen commented Jan 13, 2022

pfebrer commented Jan 13, 2022

zerothi commented Jan 28, 2022 • edited Loading

zerothi commented Jan 9, 2023

pfebrer commented Jan 9, 2023

pfebrer commented Jan 9, 2023

zerothi commented Jan 9, 2023

pfebrer commented Jan 9, 2023

zerothi commented Jan 9, 2023

zerothi commented Jul 10, 2023

zerothi commented Jul 10, 2023

pfebrer commented Jul 10, 2023

zerothi commented Jul 10, 2023

pfebrer commented Jul 10, 2023

zerothi commented Jul 10, 2023

zerothi commented Jul 16, 2023

pfebrer commented Jul 17, 2023

codecov bot commented Jan 12, 2022 •

edited

Loading

pfebrer commented Jan 13, 2022 •

edited

Loading

tfrederiksen commented Jan 13, 2022 •

edited

Loading

pfebrer commented Jan 13, 2022 •

edited

Loading

zerothi commented Jan 28, 2022 •

edited

Loading