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

ENH Add crop pipeline #247

Merged
merged 6 commits into from
May 6, 2020
Merged

ENH Add crop pipeline #247

merged 6 commits into from
May 6, 2020

Conversation

caspervdw
Copy link
Member

This adds a cropping pipeline that correctly changes the frame_shape attribute. I decided to keep the API the same as that of skimage.util.crop. There are many other pipelines that we could implement (for instance, rewrite normalize and to_rgb from pims.display, but I think it is a good idea to stay close to skimage and where possible, just import the functions instead of redoing it.

A general approach for that would be testing the function on the first frame at __init__, recording the changed attributes, and exposing them automatically. That might save us a fair amount of work, and the performance loss would be not too bad. This would be a pims functionality, as it would be specific to images (frame_shape, pixel_type, etc.).

In this case of crop, I did end up copying over the code.

As it makes use of soft-matter/slicerator#21 , we need a slicerator release to get the tests working.

@danielballan
Copy link
Member

I'll put a slicerator release on my todo list for next week unless you can get to it sooner.

@caspervdw
Copy link
Member Author

@danielballan I'll do the release now

@caspervdw
Copy link
Member Author

I would like to start adding some more pipelines, but this has to go in first
Do you agree with exposing pipelines with the same API as skimage, importing where possible?

@caspervdw caspervdw mentioned this pull request Feb 17, 2017
5 tasks
@tacaswell
Copy link
Member

What is the current state of this?

@nkeim nkeim added this to the v0.5 milestone Apr 30, 2020
@nkeim nkeim mentioned this pull request Apr 30, 2020
6 tasks
@rbnvrw
Copy link
Contributor

rbnvrw commented May 1, 2020

Hi @caspervdw, we'd like to add this very old PR to the new pims release! Do you have time to update the PR or should we do it?

@caspervdw
Copy link
Member Author

@rbnvrw I merged master and attempted to fix an incompatibility with numpy

@caspervdw
Copy link
Member Author

@rbnvrw It seems that slicerator has become incompatible with the newest numpy.

@nkeim
Copy link
Contributor

nkeim commented May 4, 2020

Thanks @caspervdw ! I'm not sure the problem is the newest numpy—our "legacy" Travis environment uses the ancient numpy 1.8.2 and still had similar errors as the other environments, such as this:

58ERROR: test_on_frame (pims.tests.test_process.TestCrop)
559----------------------------------------------------------------------
560Traceback (most recent call last):
561  File "/home/travis/build/soft-matter/pims/pims/tests/test_process.py", line 44, in test_on_frame
562    assert_equal(self.pipeline(self.rdr[0]), self.first_frame)
563  File "/home/travis/build/soft-matter/pims/pims/tests/test_process.py", line 57, in <lambda>
564    self.pipeline = lambda x: crop(x, ((5, 32-26), (7, 33-27)))
565  File "/home/travis/mc/envs/testenv/lib/python2.7/site-packages/slicerator/__init__.py", line 641, in process
566    return cls(*(tuple([a] for a in ancestors) + args), **kwargs)[0]
567  File "/home/travis/mc/envs/testenv/lib/python2.7/site-packages/slicerator/__init__.py", line 480, in __getitem__
568    return self._get(indices)
569  File "/home/travis/build/soft-matter/pims/pims/process.py", line 93, in _get
570    return np.array(ar[self._crop_slices], order=self._crop_order,
571TypeError: list indices must be integers, not tuple

I may be able to take a closer look myself. Worst case, we can just hold this PR until a minor release.

@nkeim
Copy link
Contributor

nkeim commented May 6, 2020

@caspervdw I think your commits fixed everything, except for a small mistake. The tests pass now. Could you please check that I got this right?

@caspervdw
Copy link
Member Author

Thanks for spotting that! Everything is ok now.

@caspervdw caspervdw merged commit 19ad451 into soft-matter:master May 6, 2020
@nkeim
Copy link
Contributor

nkeim commented May 6, 2020

Great! Thanks @caspervdw for writing such a useful and logical feature! I'm glad it will get released.

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

Successfully merging this pull request may close these issues.

5 participants