aniSileSiesta

[tfrederiksen]

This PR enables reading of siesta ANI files.

[zerothi]

Great addition!

It is however a generic file format and should perhaps be contained in the xyzSile then the aniSileSiesta could simply be a subclass of the xyz sile.

[codecov]

Codecov Report

Merging #544 (d7fd5af) into main (0b5164b) will increase coverage by 0.01%.
The diff coverage is 94.73%.

❗ Current head d7fd5af differs from pull request most recent head a7f81b3. Consider uploading reports for the commit a7f81b3 to get more accurate results

@@            Coverage Diff             @@
##             main     #544      +/-   ##
==========================================
+ Coverage   86.23%   86.25%   +0.01%     
==========================================
  Files         365      366       +1     
  Lines       46425    46500      +75     
==========================================
+ Hits        40036    40107      +71     
- Misses       6389     6393       +4     

Impacted Files	Coverage Δ
sisl/io/sile.py	86.72% <91.30%> (+0.39%)	⬆️
sisl/io/siesta/__init__.py	100.00% <100.00%> (ø)
sisl/io/siesta/ani.py	100.00% <100.00%> (ø)
sisl/io/tests/test_xyz.py	100.00% <100.00%> (ø)
sisl/io/xyz.py	97.26% <100.00%> (+0.15%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[tfrederiksen]

OK, do you then see the .ANI format as basically a xyzSile (but one with several entries)?

[zerothi]

Yes, that (I think) is the actual xyz file format. So for siesta we would simply do add_sile("ani", xyzSile, ...) and then don't do anything else.

[zerothi]

Yes, that (I think) is the actual xyz file format. So for siesta we would simply do add_sile("ani", xyzSile, ...) and then don't do anything else.

Well, perhaps we should do:

aniSileSiesta(xyzSile): pass

add_sile("ani", aniSileSiesta, ...)

[tfrederiksen]

What about handling the sile as something that can be iterated:

s = sisl.get_sile(anifile)
g0 = s.read_geometry()
g1 = s.read_geometry()

Right now the xyzSile forces a start from the beginning, but this could be changed.

[zerothi]

What about handling the sile as something that can be iterated:

s = sisl.get_sile(anifile)
g0 = s.read_geometry()
g1 = s.read_geometry()

Right now the xyzSile forces a start from the beginning, but this could be changed.

This should work already in the xyzSile

s = sisl.get_sile(anifile)
with s:
    g0 = s.read_geometry()
    g1 = s.read_geometry()

[tfrederiksen]

This should work already in the xyzSile

s = sisl.get_sile(anifile)
with s:
    g0 = s.read_geometry()
    g1 = s.read_geometry()

Yes, sorry, this works!

[tfrederiksen]

Am I on the right track with wrapping the single-time read_geometry function for multiple read attempts as done here?

[zerothi]

Am I on the right track with wrapping the single-time read_geometry function for multiple read attempts as done here?

Yes, I think something like this.

My idea would be to unify MD output files with a decorator, so essentially something like this:

@sile_fh_open
@read_multiple(start=0, step=None)
def read_geometry(self, atoms=None, sc=None)
...

where the decorator ensures correct calls and adds documentation etc.

def read_multiple(start=0, step=None):
    "decorate a function for doing multiple reads "
    def decorator(func):
        @wraps
        def f(*args, *, start=start, stop=stop, **kwargs):
                 if stop is None:
 return func(*args, **kwargs)
...

something like the above could work and make interfaces the same, what do you think?

[tfrederiksen]

What do you think of the current layout? I tried to follow your suggestions.

Maybe for the ANI files it would be natural to have all=True as a default?

[tfrederiksen]

How should write be handled for lists of geometries? Would it make sense that a multiple-read call returns an instance from a new class like GeometryCollection (instead of a simple list)?

[tfrederiksen]

How should write be handled for lists of geometries? Would it make sense that a multiple-read call returns an instance from a new class like GeometryCollection (instead of a simple list)?

Any thoughts on this?

[zerothi]

I would also like to remove the unnecessary commits, could you rebase and squash the commits that shouldn't be used? Then I can have a final go and get this in?

[pfebrer]

How should write be handled for lists of geometries? Would it make sense that a multiple-read call returns an instance from a new class like GeometryCollection (instead of a simple list)?

As I have said already on several other issues, I think sisl should have a well defined way of working with trajectories :) Otherwise, just reading trajectories from files as lists of Geometry objects and afterwards concatenating their coordinates in a numpy array is far from optimal. It would also be cool to add MD analysis tools into sisl, but since there's no defined way to treat trajectories, there is no space right now in the package for them, which is sad :(

[tfrederiksen]

I would also like to remove the unnecessary commits, could you rebase and squash the commits that shouldn't be used? Then I can have a final go and get this in?

I've cleaned up the commits. Let me know if you want me to do more changes, otherwise feel free to modify further.

[pfebrer]

Hi! I'm checking the sile_read_multiple function and the all argument seems odd to me. start, stop, step are already enough, no?

Why not go with the conventions of slicing in python? I.e. behaving exactly as slice.

Additionally, I know it looks a bit ugly at first sight but numpy.mgrid does something like:

gc = xyzSile(f).read_geometry[4::10]

Just something to think about, not that I personally think it's a good idea :)

[zerothi]

Hi! I'm checking the sile_read_multiple function and the all argument seems odd to me. start, stop, step are already enough, no?

See discussion here #544 (comment)