WIP: Maddie's ringfinding python modules development branch by maddiebrod · Pull Request #83 · xpdAcq/xpdtools

maddiebrod · 2019-03-07T22:20:45Z

@CJ-Wright @eaculb Let's make comments on this branch

codecov-io · 2019-03-07T22:34:57Z

Codecov Report

Merging #83 into master will decrease coverage by 11.05%.
The diff coverage is 0%.

@@             Coverage Diff             @@
##           master      #83       +/-   ##
===========================================
- Coverage   91.95%   80.89%   -11.06%     
===========================================
  Files          20       21        +1     
  Lines        1007     1178      +171     
===========================================
+ Hits          926      953       +27     
- Misses         81      225      +144

Impacted Files	Coverage Δ
xpdtools/calib2.py	`0% <0%> (ø)`
xpdtools/tests/test_tomo_tools.py	`100% <0%> (ø)`	⬆️
xpdtools/pipelines/tomo.py	`97.01% <0%> (+0.65%)`	⬆️

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 ccd5585...a1733cb. Read the comment docs.

CJ-Wright

I don't know if I fully understand how this is working, but it seems like a reasonable first pass.

CJ-Wright · 2019-03-08T19:50:38Z

xpdtools/calib2.py

+
+def findringcenter(image,thres=.20,d=20):
+
+	def findcenter(image):


Would it be possible to pull these functions outside of the main function?

CJ-Wright · 2019-03-08T20:01:33Z

xpdtools/calib2.py

+print(imarray)
+
+class Slyce():
+	def __init__(self,direction,index,imarray):


Would it be possible to use 4 spaces rather than tabs?

CJ-Wright · 2019-03-08T20:16:13Z

xpdtools/calib2.py

+
+	coords=[]
+	spread=[]
+	for row in range (int(r-3*d),int(r+3*d)):


I don't know if I understand how this works.

The for loop goes through points reasonably close to the center of the image (not necessary the rings which could pose a problem in some cases). It them finds the distance between that point and all the points that had been identified as being on the center ring. The point that is closest to being equidistant from the points on the inner ring is identified as the center point.

CJ-Wright · 2019-03-08T20:18:45Z

xpdtools/calib2.py

+imarray=tf.imread(filename)
+print(imarray)
+
+class Slyce():


Long term I'm not certain if this needs a class.

sbillinge

@maddiebrod I think it would be helpful if you could put in some one-line comments here and there thay say what the program is doing.

Also, I think we need a use-case so we can understand how it is supposed to be used. It could be something like this:

users runs calibration
xpdAcq returns image of Ni
pipeline does dark subtraction, flatfield, whatever
pipeline calls ringfinder
ringfinder finds rings
ringfinder returns approximate beam center and pixel coordinates of approximate ring positions
pipeline calls pyFai, handing it the beam center and ring position coordinates
pyFai uses these to seed the Ni calibration refinement
pipeline continues as usual.

In this scenario we do need a single function that takes in a numpy array and returns something like a dictionary of coordinates.

…umpy array as an input instead of a str

CJ-Wright

Would it be possible to pull all the classes and functions outside the findrings function?

… the main findrings function. calib4.py includes a visualization of the center point and points on rings, while calib5.py just returns the pixel indices of these points.

CJ-Wright · 2019-03-20T23:56:21Z

What are the differences among the files?

maddiebrod · 2019-03-21T00:01:51Z

calib4.py produces an image using matplotlib of the PDF with the center pixel and the pixels of the points that would be clicked in red (actually a square of pixels because one pixel is too difficult to see). This portion of code starts on line 239 of calib4.py. calib5.py is the same except it does not include the visualization.

CJ-Wright · 2019-03-21T00:18:31Z

Would it be possible to combine these files (and remove the not used ones)? You might consider putting conditionals around the visualization chunk.

…sualization portion conditional

…gfinding.py

…gs() function on 5 Ni tiff images. This version of the test function tests whether or not the x and y pixel coordinates of the center point given by findrings() are within 10 pixels of that given by paiFAI

CJ-Wright · 2019-04-11T13:23:58Z

Would you be able to use black on your code? (You can conda install it from conda-forge). It might help to make the files a bit more readable.

…ests if findrings() raises IndexError if input is 1D numpy array and one that determines if it raises an attributeError if given an input that is not a numpy array

…nput is not ndarray and modified test_ringfinding by adding a test function to test whether or not runtime error is raised when input is not ndarray

sbillinge

looks good. Are we getting close to it not being WIP any more? Do we wnat to check if it works before we merge?

sbillinge · 2019-04-16T22:04:12Z

xpdtools/tests/test_ringfinding.py

+	with pytest.raises(IndexError):
+		findrings(np.random.rand(2048))
+
+@pytest.mark.parametrize("wrong_input",[[1,2,3],3,2.7,(2,3),'banana'])


CJ-Wright

Please separate the plotting code from library code.

CJ-Wright · 2019-05-03T14:06:05Z

xpdtools/calib4.py

+import matplotlib.pyplot as plt
+
+
+class Slyce:


Maybe go with a dictionary?

slyce = {'direction': 'v', 'index': 0, 'data': array, 'pixels': []}

CJ-Wright · 2019-05-03T14:08:46Z

xpdtools/calib4.py

+
+    s = image.shape
+    # number of rows divided by 2
+    r = s[0] / 2


This might return a float, maybe use floor division?

CJ-Wright · 2019-05-03T14:11:15Z

xpdtools/calib4.py

+        slyce.pixels.append(p1)
+
+
+def finddistance(a, b):


Maybe use numpy.linalg.norm

CJ-Wright · 2019-05-03T14:15:18Z

xpdtools/calib4.py

+            self.data = list(image[self.index, :])
+
+
+def findcenter(image):


I would inline this.

CJ-Wright · 2019-05-03T14:31:56Z

xpdtools/calib4.py

+    r, c = findcenter(image)
+
+    # take 10 slices within a range of 'd' away from the cetner of the image and puts them into a list
+    slyce1 = Slyce("v", c - d, image)


slices = [] for k in ['v', 'h']: if k == 'v': row_column = c else: row_column = r for dd in [0, -d, d, -d/2, d/2]: # handle data direction here slices.append({'direction': k, 'index': row_column + dd, 'data': ...})

…intergers, got rid of the function that finds the center of the image and incorporated that part of the code directly into the findringcenter() function

…b.py

… the points on the tiff image where the findrings() function identifies the center point and a point onrings 0,1,2,5

initial commit of Maddie's ringfinding python modules

19d05eb

CJ-Wright reviewed Mar 8, 2019

View reviewed changes

sbillinge reviewed Mar 8, 2019

View reviewed changes

maddiebrod added 2 commits March 12, 2019 21:18

added comments describing what the code is doing

dd2ff3f

added docstrings to functions and modified findrings() to take in a n…

624703c

…umpy array as an input instead of a str

CJ-Wright reviewed Mar 20, 2019

View reviewed changes

calib4.py and calib5.py have the Slyce class and functions outside of…

9fdcc5c

… the main findrings function. calib4.py includes a visualization of the center point and points on rings, while calib5.py just returns the pixel indices of these points.

maddiebrod added 3 commits March 21, 2019 14:13

combined calib4.py and calib5.py into one file calib4.py, and made vi…

4d41990

…sualization portion conditional

removed filename from calib4.py and created test function in test_rin…

3447f37

…gfinding.py

modified test_ringfinding with decorator so that it tests the findrin…

b8dc4bf

…gs() function on 5 Ni tiff images. This version of the test function tests whether or not the x and y pixel coordinates of the center point given by findrings() are within 10 pixels of that given by paiFAI

maddiebrod added 2 commits April 12, 2019 21:15

modified test_ringfinding.py by adding two test functions: one that t…

d07e4fb

…ests if findrings() raises IndexError if input is 1D numpy array and one that determines if it raises an attributeError if given an input that is not a numpy array

modified findrings in calib4.py so that is raises a RuntimeError if i…

c24d4eb

…nput is not ndarray and modified test_ringfinding by adding a test function to test whether or not runtime error is raised when input is not ndarray

sbillinge reviewed Apr 16, 2019

View reviewed changes

cleaning

684e1df

CJ-Wright reviewed May 3, 2019

View reviewed changes

maddiebrod added 5 commits May 3, 2019 10:54

added extra test files in test_ringfinding.py

90a0f2c

resolved merge conflicts

acd0cba

replaced Slyce class with dictionaries, made sure r and c are always …

8c59645

…intergers, got rid of the function that finds the center of the image and incorporated that part of the code directly into the findringcenter() function

moved block of code that plots selected points to a new file vis_cali…

ba601c9

…b.py

modified vis_calib.py file so that it can take in a filename and plot…

a1733cb

… the points on the tiff image where the findrings() function identifies the center point and a point onrings 0,1,2,5


		def findringcenter(image,thres=.20,d=20):

		def findcenter(image):

		self.data = list(image[self.index, :])


		def findcenter(image):

Conversation

maddiebrod commented Mar 7, 2019

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

CJ-Wright 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!

sbillinge left a comment

Choose a reason for hiding this comment

Uh oh!

CJ-Wright left a comment

Choose a reason for hiding this comment

Uh oh!

CJ-Wright commented Mar 20, 2019

Uh oh!

maddiebrod commented Mar 21, 2019

Uh oh!

CJ-Wright commented Mar 21, 2019

Uh oh!

CJ-Wright commented Apr 11, 2019

Uh oh!

sbillinge left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CJ-Wright 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-io commented Mar 7, 2019 •

edited

Loading