WIP: improved Geometry.sort, for #191 #197

zerothi · 2020-04-01T08:08:30Z

This change allows very complicated sort calls.

ascending/descending along lattice vectors
ascending/descending along Cartesian coordinates
ascending/descending along a vector
external function to sort indices
species
group labelling
combinations (e.g. first species, then axes), this is particularly easy since Python >=3.6 uses ordered dicts for **kwargs, so sort should change interface to **kwargs
the above on a subset of atoms

All sorts are now chained (much like lexsort).
One can now do

geom.sort(axis=0, lattice=0, axis1=1)

to first sort Cartesian x, then for each subgroup in this
sort according to first lattice vector, then for each subgroup
sort according to Cartesian y.

Note how axis1 allows numeric flags to allow multiple same
sorting algorithms for the same (to achieve different order
of sort).
One can also add ascending/descending

geom.sort(axis=0, ascend=False, lattice=1, ascend=True, axis=1)

to ascend-sort x, descend-sort fa, ascend-sort y.

Also added preliminary support for sort in sgeom.

@tfrederiksen could you please have a look

zerothi · 2020-04-01T08:09:32Z

species/groups to come

codecov · 2020-04-01T08:15:20Z

Codecov Report

Merging #197 into master will increase coverage by <.01%.
The diff coverage is 74.63%.

@@            Coverage Diff             @@
##           master     #197      +/-   ##
==========================================
+ Coverage   85.23%   85.24%   +<.01%     
==========================================
  Files         120      120              
  Lines       19130    19338     +208     
==========================================
+ Hits        16306    16484     +178     
- Misses       2824     2854      +30

Impacted Files	Coverage Δ
sisl/geometry.py	`86.15% <74.63%> (-1.15%)`	⬇️
sisl/physics/electron.py	`88.75% <0%> (ø)`	⬆️
sisl/orbital.py	`98.39% <0%> (+1.51%)`	⬆️

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 a0da8c8...7209fbd. Read the comment docs.

tfrederiksen

Looks great! I have only some minor comments on the documentation.

sisl/geometry.py

tfrederiksen · 2020-04-02T11:30:22Z

sisl/geometry.py

+        A too high `atol` may have unexpected side-effects. This is because of the way
+        the sorting algorithm splits the sections for nested sorting.
+        So for coordinates with a continuous displacement the sorting may break and group
+        a large range into 1 group.
+
+        >>> a = np.arange(5) * 0.1
+        >>> (np.diff(a) > 0.05).nonzero()[0]
+        [0, 1, 2, 3]


Sorry, I do not follow this example with the a array. Can you be more explicit how it relates to the atol discussion?

I have tried to add a more reasonable explanation...

@tfrederiksen does it make sense now?

sisl/geometry.py

tfrederiksen · 2020-04-02T12:00:11Z

Note that in your opening comment this line

geom.sort(axis=0, ascend=False, lattice=1, ascend=True, axis=1)

has a clash of the repeated argument axis.

zerothi · 2020-04-02T12:26:01Z

Could you have a look again, the atol is a bit difficult to explain, for instance it would be really difficult for the sort algorithm to do a splitting of groups in twisted bilayers since there coordinates are continuous.
Using numpy lexsort forces values to be unique, my implementation does not impose this. The reason is that typically when rotating geometries one will get very tiny differences ~1e-15 which then breaks lexsort. I try to avoid this by not doing anything to coordinates that are continuously closer than atol.

For coordinates [0.1, 0.2, 0.3] and a tolerance of 0.05 one would have to split all into groups, whereas for 0.15 one could not split. One could then argue that one should split into either [0.1, 0.2] + [0.3] or [0.1] + [0.2, 0.3] but this is rather non-trivial...

tfrederiksen · 2020-04-02T12:46:01Z

Why is sgeom in.xyz --sort "axis=2;axis=1" out.xyz allowed? It appears to me that it is not generating a clash of repeated argument?

Also, with sgeom in.xyz --sort "descend;axis=2;axis=1" out.xyz it seems that both axes are sorted descending (not just the first).

zerothi · 2020-04-02T12:50:37Z

Why is sgeom in.xyz --sort "axis=2;axis=1" out.xyz allowed? It appears to me that it is not generating a clash of repeated argument?

This is because I always append an integer to keywords (so users can do what you do ;))

Also, with sgeom in.xyz --sort "descend;axis=2;axis=1" out.xyz it seems that both axes are sorted descending (not just the first).

Yes, this is by design. Any keyword has precedence until changed. The updated documentation should reflect this. So one have to disable it again, if one does not want this.

tfrederiksen · 2020-04-02T13:00:27Z

Very cool! Now I notice that indeed you write "sorting for all subsequent sorting methods" in the documentation. So the user can also do this in a script geom.sort(descend=True, axis=2, axis1=1) and achieve the same as in my sgeom command above.

zerothi · 2020-04-02T13:01:08Z

Very cool! Now I notice that indeed you write "sorting for all subsequent sorting methods" in the documentation. So the user can also do this in a script geom.sort(descend=True, axis=2, axis1=1) and achieve the same as in my sgeom command above.

Yes :)

zerothi · 2020-04-02T19:44:16Z

@tfrederiksen I have now added the remaining sortings, currently you cannot do the complicated groupings on the CLI, then one have to parse to a tuple...

You can however do the simple stuff, like doing sgeom ... --sort "group=Z", but not with optional arguments...

tfrederiksen

Beautiful!

sisl/geometry.py

CHANGELOG

sisl/geometry.py

This change allows very complicated sort calls. All sorts are now chained (much like lexsort). One can now do geom.sort(axis=0, lattice=0, axis1=1) to first sort Cartesian x, then for each subgroup in this sort according to first lattice vector, then for each subgroup sort according to Cartesian y. Note how axis1 allows numeric flags to allow multiple same sorting algorithms for the same (to achieve different order of sort). One can also add ascending/descending geom.sort(axis=0, ascend=False, lattice=1, ascend=True, axis=1) to ascend-sort x, descend-sort fa, ascend-sort y. Also added preliminary support for sort in sgeom. Signed-off-by: Nick Papior <nickpapior@gmail.com>

Now users can sort along a projection vector. This will generally not be equivalent to lattice sorting for vectors equal to lattice vectors. Only when a lattice vector is orthogonal to the others will this be true. Signed-off-by: Nick Papior <nickpapior@gmail.com>

Mainly for clarity for users Signed-off-by: Nick Papior <nickpapior@gmail.com>

Signed-off-by: Nick Papior <nickpapior@gmail.com>

…geometry A user defined atom argument did not preserve all atoms in the geometry. It now ensures that user requested atoms are sorting between them selves. All others are left untouched. I.e. sorting atom=[0, 10] will either swap 0 and 10, or do nothing. Enabled user defined functions for sorting the geometry. Signed-off-by: Nick Papior <nickpapior@gmail.com>

Signed-off-by: Nick Papior <nickpapior@gmail.com>

Sorting now allows grouping of atoms. There are 4 different groupings, Z, tag, species, symbol. This is controlled by group=str or group=(str, ...) where str is one of the "groupings" and ... is a list or list of lists with items that may be grouped together. Optionally may one of these items be None in which case it will be filled with the *rest* of the allowable searchable items. For instance geom.sort(group=('symbol', None, 'C')) first groups all non C atoms, then all C atoms. Signed-off-by: Nick Papior <nickpapior@gmail.com>

Signed-off-by: Nick Papior <nickpapior@gmail.com>

zerothi · 2020-04-03T12:07:38Z

It is in ;)

zerothi self-assigned this Apr 1, 2020

zerothi mentioned this pull request Apr 1, 2020

Geometry.sort #191

Closed

6 tasks

tfrederiksen reviewed Apr 2, 2020

View reviewed changes

tfrederiksen reviewed Apr 3, 2020

View reviewed changes

sisl/geometry.py Show resolved Hide resolved

sisl/geometry.py Outdated Show resolved Hide resolved

CHANGELOG Outdated Show resolved Hide resolved

sisl/geometry.py Show resolved Hide resolved

zerothi added 12 commits April 3, 2020 14:05

enh: enabled spaces for sgeom sort calls

e43255b

Mainly for clarity for users Signed-off-by: Nick Papior <nickpapior@gmail.com>

maint: simplified sort complexity (no need for tuples)

4ba27a9

Signed-off-by: Nick Papior <nickpapior@gmail.com>

maint: fixed a sort for 1-sized arrays

d6b81be

Signed-off-by: Nick Papior <nickpapior@gmail.com>

doc: fixed sort documentation

115504f

Signed-off-by: Nick Papior <nickpapior@gmail.com>

doc: added vector description for sort

34a7ac7

Signed-off-by: Nick Papior <nickpapior@gmail.com>

doc: fixed documentations per TF request

ed9d83e

Signed-off-by: Nick Papior <nickpapior@gmail.com>

bug: fixed sgeom --sort group=z and documentation

bcc6c51

Signed-off-by: Nick Papior <nickpapior@gmail.com>

doc: updated changelog

7209fbd

Signed-off-by: Nick Papior <nickpapior@gmail.com>

zerothi force-pushed the 191-sort branch from 4373cf0 to 7209fbd Compare April 3, 2020 12:05

zerothi merged commit fd7c041 into master Apr 3, 2020

zerothi deleted the 191-sort branch April 3, 2020 12:07

tfrederiksen mentioned this pull request Apr 3, 2020

Nanoribbon geometry #201

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: improved Geometry.sort, for #191 #197

WIP: improved Geometry.sort, for #191 #197

zerothi commented Apr 1, 2020 •

edited

Loading

zerothi commented Apr 1, 2020

codecov bot commented Apr 1, 2020 •

edited

Loading

tfrederiksen left a comment

tfrederiksen Apr 2, 2020

zerothi Apr 2, 2020

zerothi Apr 2, 2020

tfrederiksen commented Apr 2, 2020

zerothi commented Apr 2, 2020

tfrederiksen commented Apr 2, 2020

zerothi commented Apr 2, 2020

tfrederiksen commented Apr 2, 2020

zerothi commented Apr 2, 2020

zerothi commented Apr 2, 2020

tfrederiksen left a comment

zerothi commented Apr 3, 2020

WIP: improved Geometry.sort, for #191 #197

WIP: improved Geometry.sort, for #191 #197

Conversation

zerothi commented Apr 1, 2020 • edited Loading

zerothi commented Apr 1, 2020

codecov bot commented Apr 1, 2020 • edited Loading

Codecov Report

tfrederiksen left a comment

Choose a reason for hiding this comment

tfrederiksen Apr 2, 2020

Choose a reason for hiding this comment

zerothi Apr 2, 2020

Choose a reason for hiding this comment

zerothi Apr 2, 2020

Choose a reason for hiding this comment

tfrederiksen commented Apr 2, 2020

zerothi commented Apr 2, 2020

tfrederiksen commented Apr 2, 2020

zerothi commented Apr 2, 2020

tfrederiksen commented Apr 2, 2020

zerothi commented Apr 2, 2020

zerothi commented Apr 2, 2020

tfrederiksen left a comment

Choose a reason for hiding this comment

zerothi commented Apr 3, 2020

zerothi commented Apr 1, 2020 •

edited

Loading

codecov bot commented Apr 1, 2020 •

edited

Loading