Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix url with slices #3821

Merged
merged 2 commits into from
May 12, 2023
Merged

Fix url with slices #3821

merged 2 commits into from
May 12, 2023

Conversation

vallsv
Copy link
Contributor

@vallsv vallsv commented May 12, 2023

This was just not implemented.

Changelog:

Copy link
Member

@t20100 t20100 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed support of slice was missing.

Let's keep helper functions (slice_to_string and slice_sequence_to_string) private to make it clear it's not part of the API.

src/silx/io/test/test_url.py Outdated Show resolved Hide resolved
src/silx/io/url.py Outdated Show resolved Hide resolved
src/silx/io/url.py Outdated Show resolved Hide resolved
src/silx/io/url.py Show resolved Hide resolved
src/silx/io/url.py Show resolved Hide resolved
src/silx/io/test/test_url.py Show resolved Hide resolved
src/silx/io/url.py Outdated Show resolved Hide resolved
src/silx/io/url.py Show resolved Hide resolved
@vallsv vallsv force-pushed the fix-url-with-slices branch from f5a9405 to 15eefca Compare May 12, 2023 11:26
@vallsv
Copy link
Contributor Author

vallsv commented May 12, 2023

Here is with fixes.

slice_sequence_to_string is public because i need it at another place.

@t20100
Copy link
Member

t20100 commented May 12, 2023

One corner case: How does it work with () as slicing?

@vallsv
Copy link
Contributor Author

vallsv commented May 12, 2023

(basically there is the same bug in UrlSelectionTable, cause it was a copy paste, i prefer to use the same base code for now)

@vallsv
Copy link
Contributor Author

vallsv commented May 12, 2023

@t20100 what () should do? i have no idea.

@vallsv
Copy link
Contributor Author

vallsv commented May 12, 2023

Actually

  • () -> slice=
  • (1, ) -> slice=1

I suggest to rework that to have

  • () -> slice=,
  • (1, ) -> slice=1,

but i am really not sure, it's maybe better to change that in another PR

@t20100
Copy link
Member

t20100 commented May 12, 2023

what () should do?

From https://numpy.org/doc/stable/user/basics.indexing.html :

An empty (tuple) index is a full scalar index into a zero-dimensional array. x[()] returns a scalar if x is zero-dimensional and a view otherwise. On the other hand, x[...] always returns a view.

Basically it returns everything but it also works with scalars:

In [6]: s = numpy.array(1)
In [7]: s[()]
Out[7]: 1
In [8]: s[...]
Out[8]: array(1)
In [9]: s[:]
---------------------------------------------------------------------------
IndexError                                Traceback (most recent call last)
In [10]: a = numpy.array([1])
In [11]: a[:]
Out[11]: array([1])
In [12]: a[...]
Out[12]: array([1])
In [13]: a[()]
Out[13]: array([1])

@t20100
Copy link
Member

t20100 commented May 12, 2023

Acutally it should be OK as it is:
() corresponds to no slicing.

@vallsv vallsv merged commit fabfec8 into silx-kit:main May 12, 2023
@vallsv
Copy link
Contributor Author

vallsv commented May 12, 2023

Ok, nice, i prefer not to touch this semantic.

It's just that there is an inconsistency because 1 is the same as (1,), which is a bit weird. But with this 2 entries the result should also be the same with numpy slice, so i guess it's fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants