Feature/physical pixel size by toloudis · Pull Request #43 · AllenCellModeling/aicsimageio

toloudis · 2019-10-31T00:11:41Z

Another functionality patch: AICS internal code needs to be able to get the physical pixel size in x, y, and z for volume data. I implement this on the volume file format readers, and left it out of AICSImage for now. It can be accessed via the reader. As with prior PRs, this is probably subject to refactoring into a metadata wrapper class.

bonus fix for set_Description from CellProfiler/python-bioformats#112

codecov-io · 2019-10-31T00:12:57Z

Codecov Report

Merging #43 into master will increase coverage by 0.2%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master      #43     +/-   ##
=========================================
+ Coverage   87.96%   88.16%   +0.2%     
=========================================
  Files          27       27             
  Lines        1113     1132     +19     
=========================================
+ Hits          979      998     +19     
  Misses        134      134

Impacted Files	Coverage Δ
aicsimageio/readers/ome_tiff_reader.py	`89.02% <100%> (+0.56%)`	⬆️
aicsimageio/tests/readers/test_czi_reader.py	`100% <100%> (ø)`	⬆️
aicsimageio/tests/readers/test_ome_tiff_reader.py	`93.93% <100%> (+0.39%)`	⬆️
aicsimageio/readers/czi_reader.py	`97.82% <100%> (+0.26%)`	⬆️

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 45d93f4...925e8c0. Read the comment docs.

evamaxfield

Looks great. I tried out the "suggest changes" thing. Let me know if you hate them.

The only debate for me is NamedTuple, Tuple, or List.

Also, this raises codecov to 88% woooo 🎉

evamaxfield · 2019-10-31T00:34:00Z

+            return default
+        return ref.text
+
+    def get_physical_pixel_size(self, scene: int = 0):


Two nitpicky things: I personally think this should be a tuple over a list and we should have a return type hint.

Suggested change

def get_physical_pixel_size(self, scene: int = 0):

def get_physical_pixel_size(self, scene: int = 0) -> Tuple(int):

Or if we wanted to be fancy lads, a NamedTuple so that people can say im.get_physical_pixel_size().x

evamaxfield · 2019-10-31T00:34:42Z

    def get_channel_names(self, scene: int = 0):
        return self.metadata.image(scene).Pixels.get_channel_names()
+
+    def get_physical_pixel_size(self, scene: int = 0):


Same here.

Suggested change

def get_physical_pixel_size(self, scene: int = 0):

def get_physical_pixel_size(self, scene: int = 0) -> Tuple(int):

evamaxfield · 2019-10-31T00:35:04Z

+        px = float(self._getmetadataxmltext("./Metadata/Scaling/Items/Distance[@Id='X']/Value", "1.0"))
+        py = float(self._getmetadataxmltext("./Metadata/Scaling/Items/Distance[@Id='Y']/Value", "1.0"))
+        pz = float(self._getmetadataxmltext("./Metadata/Scaling/Items/Distance[@Id='Z']/Value", "1.0"))
+        return [px, py, pz]


Suggested change

return [px, py, pz]

return (px, py, pz)

evamaxfield · 2019-10-31T00:36:05Z

+
+    def get_physical_pixel_size(self, scene: int = 0):
+        p = self.metadata.image(scene).Pixels
+        return [p.get_PhysicalSizeX(), p.get_PhysicalSizeY(), p.get_PhysicalSizeZ()]


Suggested change

return [p.get_PhysicalSizeX(), p.get_PhysicalSizeY(), p.get_PhysicalSizeZ()]

return (p.get_PhysicalSizeX(), p.get_PhysicalSizeY(), p.get_PhysicalSizeZ())

evamaxfield · 2019-10-31T00:36:28Z



+@pytest.mark.parametrize("test_input, expected", [
+    (TWO_DIM_CZI, [1.0833333333333333e-06, 1.0833333333333333e-06, 1.0]),


If we switch to Tuples:

Suggested change

(TWO_DIM_CZI, [1.0833333333333333e-06, 1.0833333333333333e-06, 1.0]),

(TWO_DIM_CZI, (1.0833333333333333e-06, 1.0833333333333333e-06, 1.0)),

evamaxfield · 2019-10-31T00:36:51Z


+@pytest.mark.parametrize("test_input, expected", [
+    (TWO_DIM_CZI, [1.0833333333333333e-06, 1.0833333333333333e-06, 1.0]),
+    (SIX_DIM_CZI, [1.0833333333333333e-06, 1.0833333333333333e-06, 1e-06])


Suggested change

(SIX_DIM_CZI, [1.0833333333333333e-06, 1.0833333333333333e-06, 1e-06])

(SIX_DIM_CZI, (1.0833333333333333e-06, 1.0833333333333333e-06, 1e-06))

evamaxfield · 2019-10-31T00:37:14Z

            "ZCYX",
        ]
+        physical_pixel_sizes = [
+            [1.0833333333333333, 1.0833333333333333, 1.0],


If we switch to tuples:

Suggested change

[1.0833333333333333, 1.0833333333333333, 1.0],

(1.0833333333333333, 1.0833333333333333, 1.0),

evamaxfield · 2019-10-31T00:37:24Z

        ]
+        physical_pixel_sizes = [
+            [1.0833333333333333, 1.0833333333333333, 1.0],
+            [1.0, 1.0, 1.0],


Suggested change

[1.0, 1.0, 1.0],

(1.0, 1.0, 1.0),

evamaxfield · 2019-10-31T00:37:34Z

+        physical_pixel_sizes = [
+            [1.0833333333333333, 1.0833333333333333, 1.0],
+            [1.0, 1.0, 1.0],
+            [1.0833333333333333, 1.0833333333333333, 1.0],


Suggested change

[1.0833333333333333, 1.0833333333333333, 1.0],

(1.0833333333333333, 1.0833333333333333, 1.0),

evamaxfield · 2019-10-31T00:38:38Z


        def set_Description(self, text):
-            make_text_node(self.node, NS_SPW, "Description", test)
+            make_text_node(self.node, self.ns['spw'], "Description", text)


I think you just found a bug in OMEXML and randomly fixed it?

LOL, followed the commit message to the bioformats merge. Makes me think we should really be pulling in their omexml object every so often to update ours. Or use a git submodule?

The problem is that this version of the file has lots of mods. But this was clearly a big bug in the file that I just noticed and thought it was cheap to throw it in.

could we create a repo of our own for this that pulls from the original repo and then has a branch that integrates the diffs of what's been added so that we have a path to update it via 1 submodule PR that we would reapply every time we have to update.

evamaxfield

Thanks for changing to Tuple

heeler

Thanks for this! My one request is could we put in a TODO to refactor the vendor code into a submodule so we have a method of reapplying our changes.

heeler · 2019-10-31T19:43:52Z


        def set_Description(self, text):
-            make_text_node(self.node, NS_SPW, "Description", test)
+            make_text_node(self.node, self.ns['spw'], "Description", text)


could we create a repo of our own for this that pulls from the original repo and then has a branch that integrates the diffs of what's been added so that we have a path to update it via 1 submodule PR that we would reapply every time we have to update.

toloudis added 2 commits October 30, 2019 17:06

add get_physical_pixel_size to czi and ome tiff readers

6d91542

6863261

bonus fix for set_Description from CellProfiler/python-bioformats#112

toloudis requested review from evamaxfield and heeler October 31, 2019 00:11

evamaxfield approved these changes Oct 31, 2019

View reviewed changes

use tuple instead of list

925e8c0

evamaxfield approved these changes Oct 31, 2019

View reviewed changes

evamaxfield requested a review from AetherUnbound October 31, 2019 17:39

heeler approved these changes Oct 31, 2019

View reviewed changes

toloudis merged commit 59dd52c into master Oct 31, 2019

evamaxfield deleted the feature/physical-pixel-size branch October 31, 2019 23:02

	def get_physical_pixel_size(self, scene: int = 0):
	def get_physical_pixel_size(self, scene: int = 0) -> Tuple(int):

	return [p.get_PhysicalSizeX(), p.get_PhysicalSizeY(), p.get_PhysicalSizeZ()]
	return (p.get_PhysicalSizeX(), p.get_PhysicalSizeY(), p.get_PhysicalSizeZ())



		@pytest.mark.parametrize("test_input, expected", [
		(TWO_DIM_CZI, [1.0833333333333333e-06, 1.0833333333333333e-06, 1.0]),

	(TWO_DIM_CZI, [1.0833333333333333e-06, 1.0833333333333333e-06, 1.0]),
	(TWO_DIM_CZI, (1.0833333333333333e-06, 1.0833333333333333e-06, 1.0)),

	(SIX_DIM_CZI, [1.0833333333333333e-06, 1.0833333333333333e-06, 1e-06])
	(SIX_DIM_CZI, (1.0833333333333333e-06, 1.0833333333333333e-06, 1e-06))

	[1.0833333333333333, 1.0833333333333333, 1.0],
	(1.0833333333333333, 1.0833333333333333, 1.0),

Uh oh!

Conversation

toloudis commented Oct 31, 2019

Uh oh!

codecov-io commented Oct 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

evamaxfield left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evamaxfield left a comment

Choose a reason for hiding this comment

Uh oh!

heeler left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-io commented Oct 31, 2019 •

edited

Loading