Skip to content

Conversation

@j-c-c
Copy link
Collaborator

@j-c-c j-c-c commented Aug 31, 2021

Adds code and tests for using numpy arrays directly with ArrayImageSource.

@codecov
Copy link

codecov bot commented Aug 31, 2021

Codecov Report

Merging #449 (6cf86ac) into develop (0fd3ec9) will increase coverage by 0.63%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #449      +/-   ##
===========================================
+ Coverage    87.88%   88.51%   +0.63%     
===========================================
  Files           97      105       +8     
  Lines         6675     7944    +1269     
===========================================
+ Hits          5866     7032    +1166     
- Misses         809      912     +103     
Impacted Files Coverage Δ
src/aspire/source/image.py 95.57% <100.00%> (+0.09%) ⬆️
src/aspire/basis/__init__.py 100.00% <0.00%> (ø)
src/aspire/utils/__init__.py 100.00% <0.00%> (ø)
src/aspire/classification/rir_class2d.py 95.55% <0.00%> (ø)
src/aspire/classification/__init__.py 100.00% <0.00%> (ø)
src/aspire/numeric/complex_pca/complex_pca.py 96.15% <0.00%> (ø)
src/aspire/basis/steerable.py 91.93% <0.00%> (ø)
src/aspire/numeric/complex_pca/validation.py 42.46% <0.00%> (ø)
...rc/aspire/classification/legacy_implementations.py 100.00% <0.00%> (ø)
src/aspire/classification/class2d.py 100.00% <0.00%> (ø)
... and 9 more

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 0fd3ec9...6cf86ac. Read the comment docs.

@garrettwrong
Copy link
Collaborator

Cool so I think in tests/test_array_image_source.py, we need to add a test that exercises the new branch. It's a sort of silly test, but we mainly want to ensure we didn't typo etc in a way that would make us crash. IE, its just a smoke test. Copied from testArrayImageSource.

    def testArrayImageSourceFromNumpy(self):
        """                                                                              
        An Array can be wrapped in an ArrayImageSource when we need to deal with ImageSource objects.                                                                            
                                                                                         
        This checks round trip conversion does not crash and returns identity.
        """

        # Create an ArrayImageSource directly from Numpy array
        src = ArrayImageSource(self.ims_np)

        # Ask the Source for all images in the stack as a Numpy array
        ims_np = src.images(start=0, num=np.inf).asnumpy()  

        # Comparison should be yield identity
        self.assertTrue(np.allclose(im, self.ims_np))

I have not tested this, but I think something like this should work fine for us. @j-c-c , could you try to add this into the branch, test it, commit, and push it up?

@j-c-c j-c-c requested a review from garrettwrong September 1, 2021 19:55
Copy link
Collaborator

@garrettwrong garrettwrong left a comment

Choose a reason for hiding this comment

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

Looks good so far, thanks. We still need to:

  1. Add a test that tries to create a class with non compliant input (say a numpy array with shape (3,2,1) ), and check it returns a meaningful error message.

  2. Use this feature to simplify some of the tutorial/example code.

@garrettwrong garrettwrong linked an issue Sep 2, 2021 that may be closed by this pull request
@j-c-c j-c-c force-pushed the aspire_image_source_403 branch from 3f4085d to 6cf86ac Compare September 3, 2021 16:26
@j-c-c j-c-c requested a review from garrettwrong September 3, 2021 16:29
Copy link
Collaborator

@garrettwrong garrettwrong left a comment

Choose a reason for hiding this comment

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

Cool, I think this covers the issue. Thanks.

@j-c-c j-c-c marked this pull request as ready for review September 3, 2021 17:26
@j-c-c j-c-c requested a review from janden as a code owner September 3, 2021 17:26
Copy link
Collaborator

@janden janden left a comment

Choose a reason for hiding this comment

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

LGTM

@j-c-c j-c-c merged commit 05fce78 into develop Sep 9, 2021
@j-c-c j-c-c deleted the aspire_image_source_403 branch September 9, 2021 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ArrayImageSource should admit Arrays

4 participants