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

Add array slicing LinearOperator #162

Merged
merged 21 commits into from
Jan 12, 2022
Merged

Add array slicing LinearOperator #162

merged 21 commits into from
Jan 12, 2022

Conversation

bwohlberg
Copy link
Collaborator

Add a LinearOperator that select part of an input array via array slicing/indexing.

Is Slice the best choice of name for this operator? Perhaps Index or Select?

@bwohlberg bwohlberg added the enhancement New feature or request label Jan 8, 2022
@codecov
Copy link

codecov bot commented Jan 8, 2022

Codecov Report

Merging #162 (1632456) into main (e68cda4) will increase coverage by 0.06%.
The diff coverage is 94.52%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #162      +/-   ##
==========================================
+ Coverage   92.14%   92.20%   +0.06%     
==========================================
  Files          48       48              
  Lines        3298     3350      +52     
==========================================
+ Hits         3039     3089      +50     
- Misses        259      261       +2     
Flag Coverage Δ
unittests 92.20% <94.52%> (+0.06%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
scico/util.py 92.97% <92.85%> (-0.16%) ⬇️
scico/blockarray.py 91.20% <93.33%> (+0.36%) ⬆️
scico/linop/__init__.py 100.00% <100.00%> (ø)
scico/linop/_linop.py 97.97% <100.00%> (+0.20%) ⬆️
scico/typing.py 100.00% <100.00%> (ø)

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 e68cda4...1632456. Read the comment docs.



class Slice(LinearOperator):
"""A linear operator for slicing an array."""
Copy link
Contributor

Choose a reason for hiding this comment

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

or block array, I wonder? I think you could use one of these to select a block from a block array, but probably not use one to select a block and index within the block, if I'm reading the docs correctly.

(not saying we should add the functionality, but maybe just address this in the docstring)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think that would be worth adding. Let's discuss details.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The syntax is already established (first axis indexes the block), but this is not supported due to limitations of the relevant BlockArray code:

scico/scico/blockarray.py

Lines 799 to 807 in 445412e

def __getitem__(self, idx: Union[int, Ellipsis]) -> JaxArray:
if isinstance(idx, slice):
raise TypeError(f"Slicing not supported on block index")
if idx == Ellipsis:
return reshape(self._data, self.shape)
if idx < 0:
idx = self.num_blocks + idx
return reshape(self._data[self.bndpos[idx] : self.bndpos[idx + 1]], self.shape[idx])

As you can see, only int or Ellipsis indexing expressions are allowed, and there is no support for concatenated indexing of block and block content. It's not clear how difficult it would be to remove the former constraint (one complication would be that indexing could return another BlockArray rather than a JaxArray), but I think it would be straightforward to add support for the latter.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added support for more general indexing of BlockArray, and documented BlockArray support by Slice.


@pytest.mark.parametrize("slc", slice_examples)
def test_slice_eval(slicetestobj, slc):
x = slicetestobj.x
Copy link
Contributor

Choose a reason for hiding this comment

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

Possible alternative: remove SliceTestObj and slicetestobj, replace line 586 with
x = snp.zeros((4, 5, 6, 7), dtype=dtype), parametrize over dtype.

My eyes glaze over when I see fixtures, and what is it gaining us here? Perhaps it caches x?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I feel the same way. The approach here is a direct copy of the tests for the Sum operator. No arguments on simplifying the new tests, but it would seem to make sense to simplify the ones for Sum at the same time?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. I made an issue for fixing it, let's leave it as is for this PR.

@bwohlberg bwohlberg merged commit 86be104 into main Jan 12, 2022
@bwohlberg bwohlberg deleted the brendt/linop branch January 12, 2022 20:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants