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 in-pixel-hit-distribution for CS and eff. pitch calculation #92

Open
wants to merge 7 commits into
base: development
Choose a base branch
from

Conversation

YannickDieter
Copy link
Contributor

This adds In-pixel hit distributions for CS between 1- 4. Addidionally the calculation of the effective CS-1-pitch using the CS distribution is estimated which can be used for calculating the pointing resolution.

@coveralls
Copy link

coveralls commented Nov 17, 2017

Coverage Status

Coverage decreased (-1.06%) to 32.421% when pulling f5f8a6a on YannickDieter:development into 45d7cc4 on SiLab-Bonn:development.

@coveralls
Copy link

coveralls commented Nov 18, 2017

Coverage Status

Coverage decreased (-1.06%) to 32.421% when pulling ea7cc3d on YannickDieter:development into 8aa38ef on SiLab-Bonn:development.

Copy link
Collaborator

@DavidLP DavidLP left a comment

Choose a reason for hiding this comment

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

A useful feature but I think it can be reduced by a lot of loc.


# read track intersections wiht actual dut for cluster sizes betwenn 1 and 4
intersection_x, intersection_y, intersection_z = tracks_chunk[:]['offset_0'], tracks_chunk[:]['offset_1'], tracks_chunk[:]['offset_2']
intersection_x_cs_1 = intersection_x[np.where(tracks_chunk[:]['n_hits_dut_%i' % actual_dut] == 1)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

np.where creates new arrays and is therefore slow. We should not use it in our heavy data crunching code. Better are boolean mask arrays that create view of the original data. Thus the line should be:
intersection_x_cs_1 = intersection_x[tracks_chunk['n_hits_dut_%i' % actual_dut] == 1]
which is also better to read.

inverse=True)

if not np.allclose(intersection_z_cs_4_local[np.isfinite(intersection_z_cs_4_local)], 0.0):
print intersection_z_cs_4_local
Copy link
Collaborator

Choose a reason for hiding this comment

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

print as an instruction does not exists in Python 3

intersection_x_cs_1 = intersection_x[np.where(tracks_chunk[:]['n_hits_dut_%i' % actual_dut] == 1)]
intersection_y_cs_1 = intersection_y[np.where(tracks_chunk[:]['n_hits_dut_%i' % actual_dut] == 1)]
intersection_z_cs_1 = intersection_z[np.where(tracks_chunk[:]['n_hits_dut_%i' % actual_dut] == 1)]
intersection_x_cs_2 = intersection_x[np.where(tracks_chunk[:]['n_hits_dut_%i' % actual_dut] == 2)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Too much copy paste is a sign that it is time for a for loop and/or helper function

prealignment=prealignment,
inverse=True)

intersection_x_cs_3_local, intersection_y_cs_3_local, intersection_z_cs_3_local = geometry_utils.apply_alignment(intersection_x_cs_3, intersection_y_cs_3, intersection_z_cs_3,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Too much copy paste is a sign that it is time for a for loop and/or helper function

projection_x_1 = np.mod(intersection_x_cs_1_local, np.array([actual_pixel_size[0]] * len(intersection_x_cs_1_local)))
projection_y_1 = np.mod(intersection_y_cs_1_local, np.array([actual_pixel_size[1]] * len(intersection_y_cs_1_local)))
projection_x_2 = np.mod(intersection_x_cs_2_local, np.array([actual_pixel_size[0]] * len(intersection_x_cs_2_local)))
projection_y_2 = np.mod(intersection_y_cs_2_local, np.array([actual_pixel_size[1]] * len(intersection_y_cs_2_local)))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Too much copy paste is a sign that it is time for a for loop and/or helper function

projection_x_1_hist, edges_x = np.histogram(projection_x_1, bins=n_bins[0], range=[0.0, actual_pixel_size[0]])
projection_y_1_hist, edges_y = np.histogram(projection_y_1, bins=n_bins[1], range=[0.0, actual_pixel_size[1]])
projection_x_2_hist, _ = np.histogram(projection_x_2, bins=n_bins[0], range=[0.0, actual_pixel_size[0]])
projection_y_2_hist, _ = np.histogram(projection_y_2, bins=n_bins[1], range=[0.0, actual_pixel_size[1]])
Copy link
Collaborator

Choose a reason for hiding this comment

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

One could store these in a list and init in a loop. Much more compact

projection_y_1_hist += np.histogram(projection_y_1, bins=edges_y)[0]
projection_x_2_hist += np.histogram(projection_x_2, bins=edges_x)[0]
projection_y_2_hist += np.histogram(projection_y_2, bins=edges_y)[0]
projection_x_3_hist += np.histogram(projection_x_3, bins=edges_x)[0]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Other data structure + for-loop

@coveralls
Copy link

coveralls commented Dec 5, 2017

Coverage Status

Coverage decreased (-1.07%) to 32.67% when pulling bab1dba on YannickDieter:development into ed5def4 on SiLab-Bonn:development.

@coveralls
Copy link

coveralls commented Dec 7, 2017

Coverage Status

Coverage decreased (-1.001%) to 32.71% when pulling f0a5c69 on YannickDieter:development into a5cd072 on SiLab-Bonn:development.

Copy link
Collaborator

@DavidLP DavidLP left a comment

Choose a reason for hiding this comment

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

The coverage decreased since there is no unit test for the new function. Can you please add one?

intersections = np.column_stack((tracks_chunk[:]['offset_0'], tracks_chunk[:]['offset_1'], tracks_chunk[:]['offset_2']))

# arrays for intersections for different CS
intersections_cs_1 = np.zeros(shape=(3, len(n_hits[n_hits == 1])))
Copy link
Collaborator

Choose a reason for hiding this comment

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

For the number of entries of a numpy array the shape attribute should be used.

inverse=True)
for cs in range(4):
if use_prealignment:
intersections_cs_local[cs][0], intersections_cs_local[cs][1], intersections_cs_local[cs][2] = geometry_utils.apply_alignment(intersections_cs[cs][0], intersections_cs[cs][1], intersections_cs[cs][2],
Copy link
Collaborator

Choose a reason for hiding this comment

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

It might be beneficial here to shorten the variable names for readability and PEP8 compliance. maybe intersections_cs_local´--> inters_cs_loc

projection_y_4 = np.mod(intersection_y_cs_4_local, np.array([actual_pixel_size[1]] * len(intersection_y_cs_4_local)))

# histogram intersections and CS (for calculation of effective pitch)
projections_cs = [np.zeros_like(intersections_cs_1), np.zeros_like(intersections_cs_2),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Much better

projection_3_2d_hist, _, _ = np.histogram2d(x=projection_x_3, y=projection_y_3, bins=[edges_x, edges_y])
projection_4_2d_hist, _, _ = np.histogram2d(x=projection_x_4, y=projection_y_4, bins=[edges_x, edges_y])
for cs in range(4):
if cs == 0:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this special case handling needed? edges_x, edges_y seems constant for all cs

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

Successfully merging this pull request may close these issues.

4 participants