Slicing IO read_ routines #586

zerothi · 2023-06-16T09:16:34Z

Closes Slicing read_* methods #584
Tests added
Documentation for functionality
somewhat, more documentation would be great.
User visible changes (including notable bug fixes) are documented in CHANGELOG

codecov · 2023-06-16T09:26:59Z

Codecov Report

Merging #586 (adb10c5) into main (1dae3e0) will decrease coverage by 0.02%.
The diff coverage is 95.03%.

@@            Coverage Diff             @@
##             main     #586      +/-   ##
==========================================
- Coverage   87.30%   87.28%   -0.02%     
==========================================
  Files         374      375       +1     
  Lines       47506    47564      +58     
==========================================
+ Hits        41474    41518      +44     
- Misses       6032     6046      +14

Impacted Files	Coverage Δ
src/sisl/io/_multiple.py	`86.82% <86.82%> (ø)`
src/sisl/io/xyz.py	`95.78% <90.90%> (-1.81%)`	⬇️
src/sisl/io/sile.py	`84.82% <96.15%> (-0.68%)`	⬇️
src/sisl/io/orca/stdout.py	`97.61% <99.16%> (+1.22%)`	⬆️
src/sisl/geometry.py	`85.89% <100.00%> (+<0.01%)`	⬆️
src/sisl/io/openmx/md.py	`100.00% <100.00%> (ø)`
src/sisl/io/orca/tests/test_stdout.py	`100.00% <100.00%> (ø)`
src/sisl/io/siesta/ani.py	`100.00% <100.00%> (ø)`
src/sisl/io/siesta/tests/test_ani.py	`100.00% <100.00%> (ø)`
src/sisl/io/siesta/tests/test_stdout.py	`100.00% <100.00%> (ø)`
... and 5 more

... and 4 files with indirect coverage changes

src/sisl/io/_multiple.py

tfrederiksen

I played a bit around with this and it looks really cool. Well done!

src/sisl/io/_multiple.py

src/sisl/io/siesta/tests/test_ani.py

zerothi · 2023-06-16T11:31:21Z

I don't see a reason to not use this everywhere, i.e. read_energy etc?

Would there be objections to this?

tfrederiksen · 2023-06-16T11:37:08Z

I don't see a reason to not use this everywhere, i.e. read_energy etc?

Would there be objections to this?

I agree, no objections. I already tested this out on stdoutSileVASP.read_energy(). However, I tend to think it may cause confusion if we change the behaviour everywhere to return the first entry instead of the last (default up to now).

Maybe raise a warning for a while to make users aware that read_energy[-1]() is needed to get the last entry (that I suppose is the typical case)?

zerothi · 2023-06-16T11:42:39Z

One can do:

@SileBinder(..., default_slice=-1)
def read_energy(...)

which should ensure that it defaults to return the last one, say read_energy() will then do that.

zerothi · 2023-06-16T11:59:52Z

Where should we default to return the last? Everywhere, some of @pfebrer comments related to MD are valid.
However, I assume that those doing MD are aware of their needs and thus does require to return all.
I don't know if all should default to return the last item?

Note, that a local commit ensures that the default slice is written in the documentation of said routine, so it will automatically be added for the method.

pfebrer · 2023-06-16T12:11:17Z

Wouldn't the default slice return the confusion of the arguments we have removed? I advocate for no defaults on this, then "legacy" code will be more likely to keep doing the same thing in the future. If in the future we decide that another default is better, it will be very difficult to debug changes in the defaults in already written code.

I would just issue a warning for some time as @tfrederiksen was suggesting (perhaps keep the current API with a deprecation warning).

Also in my case I would spend more time rechecking the documentation for the defaults of each sile than simply specifying what I want 😅

pfebrer · 2023-06-16T12:17:39Z

By the way I don't know if it would be a good idea to use slices for cases where there is more than one dimension. E.g. in outSileSiesta.read_scf one could do read_scf[:, -1] meaning all MD steps, but just the last scf step.

It looks nice but it's a bit hard to know if it makes sense implementationwise because it would no longer be just a simple external iterator on the reading function.

zerothi · 2023-06-16T12:28:23Z

By the way I don't know if it would be a good idea to use slices for cases where there is more than one dimension. E.g. in outSileSiesta.read_scf one could do read_scf[:, -1] meaning all MD steps, but just the last scf step.

It looks nice but it's a bit hard to know if it makes sense implementationwise because it would no longer be just a simple external iterator on the reading function.

This is a fine idea, I think it could be implemented easily as the subsequent tuple keys are simply applied to the returned arrays. Lets see...

pfebrer · 2023-06-16T13:12:48Z

Would it be so bad to have a read_last_geometry() method if it makes sense for some Sile?

zerothi · 2023-06-16T13:15:37Z

Would it be so bad to have a read_last_geometry() method if it makes sense for some Sile?

I am pretty sure that would make sense for all siles that uses the SileBinder :)
Hmm.. Perhaps you are right.

Ok, so default to only the next item, as it is now. The default_slice should then preferentially be used by extensions not in sisl.

tfrederiksen · 2023-06-16T13:19:39Z

Would it be so bad to have a read_last_geometry() method if it makes sense for some Sile?

I think this is a good suggestion, making it explicit for the user what is looked up. In particular the warning could suggest users to adapt read_last_energy() in their scripts.

pfebrer · 2023-06-16T13:20:32Z

I am pretty sure that would make sense for all siles that uses the SileBinder

Yeah maybe 😅

I guess if read_* functions only parse lines it will be also easier to combine them. For example you could implement something to read geometries and energies in the same pass through the file.

zerothi · 2023-06-16T13:20:49Z

Would it be so bad to have a read_last_geometry() method if it makes sense for some Sile?

I think this is a good suggestion, making it explicit for the user what is looked up. In particular the warning could suggest users to adapt read_last_energy() in their scripts.

If all this is for explicitness, wouldn't the current availability of read_energy[-1] be fine? Is this so bad to ask users to do as compared to read_energy_last?

zerothi · 2023-06-16T18:14:03Z

I have thought about it, and I have now added this, what do you think?

read_geometry.last(*args, **kwargs)
read_geometry.next(*args, **kwargs)

to explicitly request the next item, and the last item. Remark that first would have been misleading as in the case of @pfebrer's case above.

pfebrer · 2023-06-16T18:47:55Z

Sounds good!

But what does read_geometry() do?

zerothi · 2023-06-16T18:48:41Z

Sounds good!

But what does read_geometry() do?

same as next so I don't know if that is needed? but more for clarity, as you suggested.

pfebrer · 2023-06-16T18:55:34Z

Hmm it is very difficult to know what users will think about it. But it doesn't seem it could be harmful in the future to maintain these two properties (fingers crossed haha) so we could have them both.

zerothi · 2023-06-17T06:11:11Z

@tfrederiksen you might be interested in the latest commit which changes the orca output to follow this method. I couldn't really safe the all argument, but I hope this is fine. In the end when we get to a release it will be 0.14. Let me know your thoughts.

Probably this should go in ASAP, and then the other siles will come as required.

src/sisl/io/orca/stdout.py

-        if len(blocks) > 0:
-            return blocks[-1]
-        return None
+        return read_block(step_to)


src/sisl/io/orca/stdout.py

-        if len(blocks) > 0:
-            return blocks[-1]
-        return None
+        return read_block(step_to)


pfebrer · 2023-06-17T07:02:44Z

Could read_energy and similars return a numpy array by default instead of a Collection?

zerothi · 2023-06-17T07:35:13Z

Could read_energy and similars return a numpy array by default instead of a Collection?

That would be hard for end users, then the users need to read which indices corresponds to which energies, that was why the property dict was used.
It would be nice if they all allow collection or dataframes.

tfrederiksen

I think this looks great. After merge I will revisit #573.

Fixed documentation issues and enabled this. Storing sliced objects will also work. Currently nested slicing is not possible. enabled nested with of a sile and changed to SileBinder The sile_read_multiple has been deleted, now the way to do it is SileBinder. This is more versatile and allows better handling. It also patches the documentation. This changes the behaviour of all files using this. By default a read_* with the SileBinder will only read the next entry, however to read multiple one should use read_*[...] for slicing the data. The bound method will try and be smart about only reading the needed elements, there is a bit of overhead, but largely it should be fast. All tests are functional again. Signed-off-by: Nick Papior <[email protected]>

if the file reads raises something, then announce Signed-off-by: Nick Papior <[email protected]>

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

This makes the read-functionalities more useable to retain other parts of the information. All tests are functional again. Signed-off-by: Nick Papior <[email protected]>

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

Added lots of typehints in the code, primarily in the io code base. Changed the way the siesta.stdout siles handle data. They now rely on the SileSlicer and thus can be sliced upon calling. It makes the code a little simpler but also highligted some problems. In particular the problem arise when the return function needs to do different things depending on whether the file is done reading, or not. The defaults for stdoutSileSiesta has changed to read the next entry in the file. This is in contrast to the way it was. It will require users to adapt! the reason for this can be found in the discussion in #586. This streamlines the usage across the different parts of the IO handling. Signed-off-by: Nick Papior <[email protected]>

zerothi force-pushed the 584-sile-slicer branch from 2182392 to 49c43b8 Compare June 16, 2023 09:23

github-advanced-security bot found potential problems Jun 16, 2023

View reviewed changes

src/sisl/io/_multiple.py Fixed Show fixed Hide fixed

src/sisl/io/_multiple.py Fixed Show fixed Hide fixed

tfrederiksen reviewed Jun 16, 2023

View reviewed changes

src/sisl/io/_multiple.py Show resolved Hide resolved

src/sisl/io/siesta/tests/test_ani.py Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

github-advanced-security bot found potential problems Jun 17, 2023

View reviewed changes

tfrederiksen approved these changes Jun 20, 2023

View reviewed changes

zerothi added 7 commits June 20, 2023 10:11

removed try..except

5203e9d

if the file reads raises something, then announce Signed-off-by: Nick Papior <[email protected]>

added negative test and reversing elements

3665e05

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

added default slice to the patched documentation

1a56852

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

added last/next in the slicer

0552845

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

converted orca.stdout to use SileBinder

6eb605c

This makes the read-functionalities more useable to retain other parts of the information. All tests are functional again. Signed-off-by: Nick Papior <[email protected]>

removed parameters from xyz

34a265e

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

zerothi force-pushed the 584-sile-slicer branch from adb10c5 to 34a265e Compare June 20, 2023 08:12

zerothi merged commit 0b17eec into main Jun 20, 2023
3 of 4 checks passed

zerothi deleted the 584-sile-slicer branch June 20, 2023 08:12

zerothi mentioned this pull request Mar 1, 2024

added typing in lots of io places, added SileSlicer #695

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Slicing IO read_ routines #586

Slicing IO read_ routines #586

zerothi commented Jun 16, 2023 •

edited

Loading

codecov bot commented Jun 16, 2023 •

edited

Loading

tfrederiksen left a comment

zerothi commented Jun 16, 2023

tfrederiksen commented Jun 16, 2023

zerothi commented Jun 16, 2023 •

edited

Loading

zerothi commented Jun 16, 2023 •

edited

Loading

pfebrer commented Jun 16, 2023 •

edited

Loading

This comment was marked as resolved.

pfebrer commented Jun 16, 2023

This comment was marked as resolved.

This comment was marked as resolved.

zerothi commented Jun 16, 2023

This comment was marked as resolved.

This comment was marked as resolved.

pfebrer commented Jun 16, 2023

zerothi commented Jun 16, 2023

tfrederiksen commented Jun 16, 2023

pfebrer commented Jun 16, 2023

zerothi commented Jun 16, 2023

zerothi commented Jun 16, 2023

pfebrer commented Jun 16, 2023

zerothi commented Jun 16, 2023

pfebrer commented Jun 16, 2023 •

edited

Loading

zerothi commented Jun 17, 2023

pfebrer commented Jun 17, 2023

zerothi commented Jun 17, 2023

tfrederiksen left a comment

Slicing IO read_ routines #586

Slicing IO read_ routines #586

Conversation

zerothi commented Jun 16, 2023 • edited Loading

codecov bot commented Jun 16, 2023 • edited Loading

Codecov Report

tfrederiksen left a comment

Choose a reason for hiding this comment

zerothi commented Jun 16, 2023

tfrederiksen commented Jun 16, 2023

zerothi commented Jun 16, 2023 • edited Loading

zerothi commented Jun 16, 2023 • edited Loading

pfebrer commented Jun 16, 2023 • edited Loading

This comment was marked as resolved.

pfebrer commented Jun 16, 2023

This comment was marked as resolved.

This comment was marked as resolved.

zerothi commented Jun 16, 2023

This comment was marked as resolved.

This comment was marked as resolved.

pfebrer commented Jun 16, 2023

zerothi commented Jun 16, 2023

tfrederiksen commented Jun 16, 2023

pfebrer commented Jun 16, 2023

zerothi commented Jun 16, 2023

zerothi commented Jun 16, 2023

pfebrer commented Jun 16, 2023

zerothi commented Jun 16, 2023

pfebrer commented Jun 16, 2023 • edited Loading

zerothi commented Jun 17, 2023

pfebrer commented Jun 17, 2023

zerothi commented Jun 17, 2023

tfrederiksen left a comment

Choose a reason for hiding this comment

zerothi commented Jun 16, 2023 •

edited

Loading

codecov bot commented Jun 16, 2023 •

edited

Loading

zerothi commented Jun 16, 2023 •

edited

Loading

zerothi commented Jun 16, 2023 •

edited

Loading

pfebrer commented Jun 16, 2023 •

edited

Loading

pfebrer commented Jun 16, 2023 •

edited

Loading