enabled ellipsis here and there

[zerothi]

Should be improved so None fully gets removed.

[codecov]

Codecov Report

Attention: Patch coverage is 83.33333% with 3 lines in your changes missing coverage. Please review.

Project coverage is 86.85%. Comparing base (42680ae) to head (e20395a).
Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
src/sisl/io/siesta/stdout.py	60.00%	2 Missing ⚠️
src/sisl/_core/geometry.py	75.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #863      +/-   ##
==========================================
- Coverage   86.85%   86.85%   -0.01%     
==========================================
  Files         405      405              
  Lines       53038    53055      +17     
==========================================
+ Hits        46068    46082      +14     
- Misses       6970     6973       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[pfebrer]

Are you sure you want to do this? This is not standard behavior. Numpy for example uses ellipsis to indicate, "for all other axes do nothing". I.e. it is not something that applies for a single axis.

[zerothi]

Are you sure you want to do this? This is not standard behavior. Numpy for example uses ellipsis to indicate, "for all other axes do nothing". I.e. it is not something that applies for a single axis.

But right now read_scf(iscf=None) quite unintuitively means all iscf elements, I can see that it isn't standard, but on the other hand, iscf is definitely (and will always be) a single dimension, so ... is clear IMHO. Using Opt.ALL is also annoying, using some weird constant for obtaining this.

So before one would do:

read_scf(iscf=None)
read_scf(iscf=slice(None))

Why would read_scf(iscf=...) be so far from standard behaviour?

The standard says:

The same as the ellipsis literal “...”. Special value used mostly in conjunction with extended slicing syntax for user-defined container data types.

which I kind of think is ok here? But I would be happy to hear why not! ;)

[pfebrer]

I have to admit when I say standard I mean numpy, but I would assume people follows numpy's conventions when it comes to array slicing.

In particular:

array[..., 0]

means:

• numpy: Give me the first element of the last dimension, regardless of how many dimensions there are.
• This PR: Give me the first element of the second dimension.

It is true that for now we don't support multiple dimensions, but this is where it seems the implementation would go if we supported them.

I am ok with using the ellipsis as arguments of the function call (although I don't see the problem with using None), but I would not support it in __getitem__, where [:] is much more natural.

[zerothi]

I have to admit when I say standard I mean numpy, but I would assume people follows numpy's conventions when it comes to array slicing.

In particular:

array[..., 0]

means:

* `numpy`: Give me the first element of the last dimension, regardless of how many dimensions there are.

* This PR: Give me the first element of the first dimension.

I see, I think mainly this would be useful in the iscf arguments, so probably I should just remove it from the SileBinder as you say.

It is true that for now we don't support multiple dimensions, but this is where it seems the implementation would go if we supported them.

I am ok with using the ellipsis as arguments of the function call (although I don't see the problem with using None), but I would not support it in __getitem__, where [:] is much more natural.

Agreed, getitem is simpler with :. I can remove that here, but for iscf=... I think this reads more natural than iscf=None, no?

Let me fix it...

[zerothi]

So, would only for iscf arguments be fine?

[pfebrer]

Agreed, getitem is simpler with :. I can remove that here, but for iscf=... I think this reads more natural than iscf=None, no?

I guess it depends on the person, I am used to None meaning "all". For example, whenever there is an atoms argument, atoms=None means "all atoms". But it could be that ... feels more natural for some people. It does not hurt to support ... if you think it is more natural, but I would keep both if possible (?)

[zerothi]

Yeah, for sure we need to have them both.
So would you think adding atoms=... would be bad? 🤔 😜

[pfebrer]

For me it would be unnecessary :)

[pfebrer]

(no one has ever complained about it)

[zerothi]

To contest the numpy default.

This means everything:

array[...] = 0.

so I would think that using ellipsis for everything in these arguments would make sense.

I have now amended so atoms and orbitals arguments can be ... to mean all.