Function to build heteroribbons

[pfebrer]

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]

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[bot]]

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

[tfrederiksen]

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]

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]

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]

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]

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:

[zerothi]

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]

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]

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]

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]

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

[zerothi]

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]

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]

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

[zerothi]

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

[pfebrer]

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

[tfrederiksen]

• 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]

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]

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]

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]

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]

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]

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]

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

[pfebrer]

And which shifts are valid depends on the exact characteristics of the junction, it is not straightforward

[tfrederiksen]

@pfebrer , what about a quick chat on discord now?

[pfebrer]

After talking with @tfrederiksen, I think the best way to go is to let users choose if they want the "supervised" version that makes sure you never leave dangling bonds or a "grown up" version that just lets you do whatever you want by shifting always (shift * half_lattice).

This would be switched on/off with a flag e.g. graphene_nanoribbon([...], supervised=False).

Do you agree @zerothi?

[zerothi]

After talking with @tfrederiksen, I think the best way to go is to let users choose if they want the "supervised" version that makes sure you never leave dangling bonds or a "grown up" version that just lets you do whatever you want by shifting always (shift * half_lattice).

This would be switched on/off with a flag e.g. graphene_nanoribbon([...], supervised=False).

Do you agree @zerothi?

Sounds good to me. However, just be reading the name, I can't clarify its intent. Could we name it:

heteroribbon(..., allow_dangling=False|True)

for clarity? Or whatever seems appropriate?

[pfebrer]

By the way in the "not supervised" version what would it be better:

• 0 shift means the bottom atom at the same position as the previous section.
• Alignment is with respect to a fixed reference (as in Thomas implementation).

?

Could we name it:

Yes, sure!

(although allow_dangling sounds like you raise errors on dangling, but it's more than that. We will find the right name :))