Pipeline Components: LearnTransform and ApplyTransform #1083

shanaxel42 · 2019-03-14T18:42:55Z

This PR adds two new pipeline components LearnTransform, and ApplyTransform replacing our existing Registration component.

LearnTransform currently only provides one method, Translation that computes translation shifts on an imagestack given a reference image and stores them as a new object, TransformsList.
ApplyTransform also has one method Warp; that takes an imagestack and a TransformsList, applies the transforms to each axes subset described, and returns a transformed Imagstack.
In the ISS pipeline Fourier Registration was replaced with this Learn/Apply series in the notebook and cli test.
Fourier Registration was deleted.

codecov-io · 2019-03-14T18:46:46Z

Codecov Report

Merging #1083 into master will increase coverage by 0.33%.
The diff coverage is 98.61%.

@@            Coverage Diff             @@
##           master    #1083      +/-   ##
==========================================
+ Coverage   89.89%   90.23%   +0.33%     
==========================================
  Files         120      125       +5     
  Lines        4414     4637     +223     
==========================================
+ Hits         3968     4184     +216     
- Misses        446      453       +7

Impacted Files	Coverage Δ
starfish/pipeline/algorithmbase.py	`97.72% <ø> (ø)`	⬆️
starfish/test/full_pipelines/cli/test_iss.py	`100% <ø> (ø)`	⬆️
starfish/types/__init__.py	`100% <ø> (ø)`	⬆️
starfish/types/_constants.py	`97.91% <100%> (+0.09%)`	⬆️
starfish/test/full_pipelines/api/test_iss_api.py	`100% <100%> (ø)`	⬆️
starfish/image/_learn_transform/transforms_list.py	`100% <100%> (ø)`
...rfish/test/image/registration/test_registration.py	`100% <100%> (ø)`
starfish/image/_apply_transform/__init__.py	`100% <100%> (ø)`
starfish/image/_learn_transform/__init__.py	`100% <100%> (ø)`
starfish/image/__init__.py	`100% <100%> (ø)`	⬆️
... and 15 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 c8b6801...fb0657a. Read the comment docs.

ttung · 2019-03-20T18:45:50Z

starfish/image/_apply_transform/warp.py

+    """Class that applies a list of arbitrary skimage GeometricTransforms to an ImageStack
+     using skimage.transform.warp"""
+
+    def __init__(self, transforms_list: Union[str, TransformsList]):


things that can be loaded from a file should be passed into run(..). Furthermore, the loading from a file should happen in the _cli method.

ttung · 2019-03-21T17:35:32Z

starfish/image/_apply_transform/warp.py

+        if not in_place:
+            # create a copy of the ImageStack, call apply on that stack with in_place=True
+            image_stack = deepcopy(stack)
+            return self.run(


nit: you could probably unwrap

ttung · 2019-03-21T21:58:27Z

starfish/image/_apply_transform/warp.py

+        Parameters
+        ----------
+        transforms_list: TransformsList
+            A list of tuples that describe a subset of an ImageStack axis and a GeometricTransform


Suggested change

A list of tuples that describe a subset of an ImageStack axis and a GeometricTransform

A list of tuples. Each tuple consists of an ImageStack selector and a GeometricTransform

ttung · 2019-03-21T22:01:28Z

starfish/image/_apply_transform/warp.py

+        ----------
+        transforms_list: TransformsList
+            A list of tuples that describe a subset of an ImageStack axis and a GeometricTransform
+            to apply to it.


Suggested change

to apply to it.

to apply to the portion of the image identified by the selector.

ttung · 2019-03-21T22:03:14Z

starfish/image/_learn_transform/transforms_list.py

+from starfish.types import Axes
+
+
+class TransformsList:


Is this to be the canonical file format for transforms? Is it generic enough?

It may be worth asking whether this will need to be versioned.

ttung · 2019-03-22T03:43:47Z

starfish/image/_learn_transform/translation.py

+    def run(self, stack: ImageStack) -> TransformsList:
+        """
+        Iterate over the given axis of an ImageStack and learn the Similarity transform
+         based off the instantiated reference_image.


Suggested change

based off the instantiated reference_image.

based off the instantiated reference_image.

ttung · 2019-03-22T03:44:44Z

starfish/image/_learn_transform/translation.py

+            if len(target_image.shape) != 2:
+                raise ValueError(
+                    "Only axes: " + self.axis.value + " can have a length > 1, "
+                                                      "please max project."


can this be a pointer to the max projection filter?

ttung · 2019-03-22T03:44:57Z

starfish/image/_learn_transform/translation.py

+                                                           upsample_factor=self.upsampling)
+            print(f"For {self.axis}: {a}, Shift: {shift}, Error: {error}")
+            selectors = {self.axis: a}
+            # reverse shift because SimilarityTransform stores in y,x format


ttung · 2019-03-22T03:45:07Z

starfish/image/_learn_transform/translation.py

+            shift, error, phasediff = register_translation(src_image=target_image,
+                                                           target_image=reference_image,
+                                                           upsample_factor=self.upsampling)
+            print(f"For {self.axis}: {a}, Shift: {shift}, Error: {error}")


do we want to gate this on verbose?

starfish/pipeline/algorithmbase.py

…ase (#1095) Rather than each specific PipelineComponent have a link to the AlgorithmBase that describes the contract that the algorithm must implement, each AlgorithmBase implementation points to the PipelineComponent that it serves. The current arrangement forces all the algorithms to be defined and evaluated in the interpreter before the pipeline component is defined and evaluated. If we want to create an algorithm that's only used in tests, this is not possible. The new arrangement ensures that any implementation of a specific AlgorithmBase is registered with its corresponding PipelineComponent. This adds explicit abstract classes to the AlgorithmBase class hierarchy to make it easier to detect what's an actual algorithm implementation. Depends on #1094 Test plan: `make -j fast`

ttung · 2019-03-28T23:29:28Z

Makefile

@@ -29,7 +29,7 @@ all:	fast

 ### UNIT #####################################################
 #
-fast:	lint mypy fast-test docs-html
+fast:	lint mypy fast-test clean-docs docs-html


this is from #1100?

ttung · 2019-03-28T23:30:31Z

docs/source/api/image/index.rst

@@ -10,8 +10,5 @@ Image Manipulation
 .. toctree::
   filtering/index.rst

-.. toctree::


We probably need a new doc sections for LearnTransform and ApplyTransform.

ttung · 2019-03-28T23:38:30Z

starfish/image/_apply_transform/_base.py

+        transformed.export(output)
+
+    @staticmethod
+    @click.group("apply_transform")


Suggested change

@click.group("apply_transform")

@click.group(COMPONENT_NAME)

ttung · 2019-03-28T23:40:46Z

starfish/image/_apply_transform/warp.py

+            if True, process ImageStack in-place, otherwise return a new stack
+        verbose : bool
+            if True, report on filtering progress (default = False)
+        transforms_list: TransformsList


can you reorder this to before in_place?

ttung · 2019-03-28T23:41:12Z

starfish/image/_learn_transform/_base.py



-COMPONENT_NAME = "registration"
+COMPONENT_NAME = "lean_transform"


Suggested change

COMPONENT_NAME = "lean_transform"

COMPONENT_NAME = "learn_transform"

ttung · 2019-03-29T17:28:46Z

starfish/image/_learn_transform/transforms_list.py

+               transform_object: GeometricTransform
+               ) -> None:
+        """
+        Adds a new TransformationObject to the list


TransformationObject is not a thing, is it?

I just meant it to refer to the GeometricTransform class and subclasses, I'll make it more clear though

ttung · 2019-03-29T17:34:14Z

starfish/image/_learn_transform/translation.py

+        reference_stack: ImageStack
+            The target image used in skimage.feature.register_translation
+        upsampling: int
+            upsampling factor, defualt 1


Suggested change

upsampling factor, defualt 1

upsampling factor (default=1). See http://scikit-image.org/docs/dev/api/skimage.feature.html#skimage.feature.register_translation for an explanation of this parameter.

ttung · 2019-03-29T17:35:19Z

starfish/image/_learn_transform/translation.py

+    def run(self, stack: ImageStack) -> TransformsList:
+        """
+        Iterate over the given axes of an ImageStack and learn the Similarity transform
+        based off the instantiated reference_image.


reference_stack?

how about:

Suggested change

based off the instantiated reference_image.

based off the reference_stack passed into :py:class:`Translation`'s constructor.

ttung · 2019-03-29T17:36:05Z

starfish/image/_learn_transform/translation.py

+            target_image = np.squeeze(stack.sel({self.axes: a}).xarray)
+            if len(target_image.shape) != 2:
+                raise ValueError(
+                    "Only axes: " + self.axes.value + " can have a length > 1, "


use f-strings:

Suggested change

"Only axes: " + self.axes.value + " can have a length > 1, "

f"Only axes: {self.axes.value} can have a length > 1, please use the MaxProj filter."

ttung · 2019-03-29T17:36:29Z

starfish/image/_learn_transform/translation.py

+            shift, error, phasediff = register_translation(src_image=target_image,
+                                                           target_image=reference_image,
+                                                           upsample_factor=self.upsampling)
+            print(f"For {self.axes}: {a}, Shift: {shift}, Error: {error}")


intended? should this be gated on verbose?

This was in the old fourier registration class so I kept it for continuity. @ambrosejcarr still needed?

Should be gated. Always hated this. 😆

Co-Authored-By: shanaxel42 <shannon.axelrod@chanzuckerberg.com>

ambrosejcarr

This is great work. Thank you for this! I made a few suggestions but should be fine to merge when resolved.

ambrosejcarr · 2019-04-02T01:33:39Z

docs/source/index.rst

@@ -59,7 +59,6 @@ To see the code or report a bug, please visit the `github repository
  <div class="col-md-3">
  <h2>Features</h2>

-* Registration: :ref:`API <registration>`


Would it make sense to link to the newly created docs for learning and applying transformations?

ambrosejcarr · 2019-04-02T01:39:05Z

starfish/image/_apply_transform/warp.py

+        in_place : bool
+            if True, process ImageStack in-place, otherwise return a new stack
+        verbose : bool
+            if True, report on filtering progress (default = False)


Suggested change

if True, report on filtering progress (default = False)

if True, report on transformation progress (default = False)

ambrosejcarr · 2019-04-02T01:47:26Z

starfish/image/_learn_transform/transforms_list.py

+    objects to apply to an Imagestack"""
+
+    def __init__(self,
+                 transforms_list: List[Tuple[Mapping[Axes, int],


Suggest a top-level NamedTuple for this:

TransformsList(NamedTuple): axes: Mapping[Axes, int] # axes this transformation modifies transform_type: TransformType # one of <relevant skimage docs> transform: GeometricTransform # dunno what to write here

It would also simplify the append function.

ambrosejcarr · 2019-04-02T01:50:17Z

starfish/image/_learn_transform/transforms_list.py

+    def from_json(cls, url_or_path: str) -> "TransformsList":
+        """
+        Load a TransformsList from a json file or a url pointing to such a file
+        Loads configuration from :py:class:`starfish.config.StarfishConfig`


What's the relevance of configuration for this function? I don't think the user will know.

ambrosejcarr · 2019-04-02T01:54:26Z

starfish/image/_learn_transform/translation.py

+
+    def run(self, stack: ImageStack, *args) -> TransformsList:
+        """
+        Iterate over the given axes of an ImageStack and learn the Similarity transform


Should note that it only supports 2-d data at this time, somewhere.

ambrosejcarr · 2019-04-02T01:55:04Z

starfish/image/_learn_transform/translation.py

+            shift, error, phasediff = register_translation(src_image=target_image,
+                                                           target_image=reference_image,
+                                                           upsample_factor=self.upsampling)
+            print(f"For {self.axes}: {a}, Shift: {shift}, Error: {error}")


Should be gated. Always hated this. 😆

ambrosejcarr · 2019-04-02T01:58:07Z

starfish/image/_learn_transform/translation.py

+        return transforms
+
+    @staticmethod
+    @click.command("Translation")


On theme with our crusade against string constants, does the following work?

Suggested change

@click.command("Translation")

@click.command(Translation.__name__)

No, that doesn't, because you're still defining the class. :(

ambrosejcarr · 2019-04-02T01:58:44Z

starfish/test/full_pipelines/api/test_iss_api.py

-    assert np.array_equal(genes, np.array(['ACTB', 'CD68', 'CTSL2', 'EPCAM', 'ETV4', 'GAPDH',
-                                           'HER2', 'MET', 'RAC1', 'TFRC', 'TP53', 'VEGF']))
-    assert np.array_equal(gene_counts, [18, 1, 5, 2, 1, 12, 3, 1, 2, 1, 1, 2])
+    assert np.array_equal(genes, np.array(['ACTB', 'CD68', 'CTSL2', 'EPCAM',


Lol. I guess I'm cool with this. 👍

This is probably our fault for using a fragile spot detector.

ttung · 2019-04-02T06:25:27Z

starfish/test/full_pipelines/api/test_iss_api.py

-    assert pipeline_log[1]['method'] == 'FourierShiftRegistration'
-    assert pipeline_log[2]['method'] == 'BlobDetector'
+    assert pipeline_log[1]['method'] == 'Warp'
+    assert pipeline_log[3]['method'] == 'BlobDetector'


Um... what's step 2?

ttung · 2019-04-02T06:25:54Z

starfish/test/image/registration/test_registration.py

+    exp = data.ISS(use_test_data=True)
+    stack = exp.fov().get_image('primary')
+    reference_stack = exp.fov().get_image('dots')
+    translation = Translation(reference_stack=reference_stack,


nit: unwrap

…to saxelrod-translation

starfish/test/full_pipelines/cli/test_iss.py changed in #1083, but docs/source/usage/iss/iss_cli.sh was never updated.

Shannon Axelrod added 9 commits March 11, 2019 10:39

changes.

cbb88e0

things working but neesds work

a2b9ff7

replaced iss but tests failing

25e1d4b

fixing most lint errors

96f0ec3

things working correctly, need to serialize/deserialize transforms_list

f1e1f30

created class for transforms list

eba779f

cli working, need better tests

393f2b9

better tests

1357c66

better documentation

c3de8de

shanaxel42 requested review from ambrosejcarr and ttung March 14, 2019 18:49

Shannon Axelrod added 5 commits March 14, 2019 14:00

fixing build error

b08f0b3

changes.

27dcc5c

fixing docs build

fe17786

reverting index.rst file

0071ea9

regenerated notebooks

9ee5eec

ttung requested changes Mar 22, 2019

View reviewed changes

ttung and others added 4 commits March 22, 2019 09:54

adding type to transforms list

8ba1c7e

merge from master

776bd19

review changes?

1820bdd

shanaxel42 requested a review from ttung March 22, 2019 21:24

Shannon Axelrod added 3 commits March 22, 2019 16:20

refactoring transforms-list to be a part of run

0afd305

regenerated notebooks

45fc168

fixing tests

6f75782

ttung requested changes Mar 29, 2019

View reviewed changes

Shannon Axelrod and others added 3 commits April 1, 2019 09:56

review changes

0bfd00e

merge from master

d071c8d

Update starfish/pipeline/algorithmbase.py

17d581e

Co-Authored-By: shanaxel42 <shannon.axelrod@chanzuckerberg.com>

shanaxel42 requested a review from ttung April 1, 2019 17:20

ambrosejcarr approved these changes Apr 2, 2019

View reviewed changes

ttung reviewed Apr 2, 2019

View reviewed changes

ttung approved these changes Apr 2, 2019

View reviewed changes

Shannon Axelrod added 2 commits April 2, 2019 10:29

review changes

53c278c

Merge branch 'saxelrod-translation' of github.com:spacetx/starfish in…

fb0657a

…to saxelrod-translation

shanaxel42 merged commit b168903 into master Apr 2, 2019

shanaxel42 deleted the saxelrod-translation branch April 2, 2019 18:44

ttung mentioned this pull request Apr 6, 2019

Update iss_cli.sh to reflect the contents of test_iss.py #1141

Merged

ttung pushed a commit that referenced this pull request Apr 8, 2019

Update iss_cli.sh to reflect the contents of test_iss.py

c718b56

starfish/test/full_pipelines/cli/test_iss.py changed in #1083, but docs/source/usage/iss/iss_cli.sh was never updated.

ttung added a commit that referenced this pull request Apr 8, 2019

Update iss_cli.sh to reflect the contents of test_iss.py (#1141)

b9ae91c

starfish/test/full_pipelines/cli/test_iss.py changed in #1083, but docs/source/usage/iss/iss_cli.sh was never updated.

	A list of tuples that describe a subset of an ImageStack axis and a GeometricTransform
	A list of tuples. Each tuple consists of an ImageStack selector and a GeometricTransform

	to apply to it.
	to apply to the portion of the image identified by the selector.

	based off the instantiated reference_image.
	based off the instantiated reference_image.



		COMPONENT_NAME = "registration"
		COMPONENT_NAME = "lean_transform"

	COMPONENT_NAME = "lean_transform"
	COMPONENT_NAME = "learn_transform"

	upsampling factor, defualt 1
	upsampling factor (default=1). See http://scikit-image.org/docs/dev/api/skimage.feature.html#skimage.feature.register_translation for an explanation of this parameter.

	based off the instantiated reference_image.
	based off the reference_stack passed into :py:class:`Translation`'s constructor.

	"Only axes: " + self.axes.value + " can have a length > 1, "
	f"Only axes: {self.axes.value} can have a length > 1, please use the MaxProj filter."

	if True, report on filtering progress (default = False)
	if True, report on transformation progress (default = False)

	@click.command("Translation")
	@click.command(Translation.__name__)

Pipeline Components: LearnTransform and ApplyTransform #1083

Pipeline Components: LearnTransform and ApplyTransform #1083

Conversation

shanaxel42 commented Mar 14, 2019 • edited Loading

codecov-io commented Mar 14, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ambrosejcarr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shanaxel42 commented Mar 14, 2019 •

edited

Loading

codecov-io commented Mar 14, 2019 •

edited

Loading