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

Add geometry methods to get physical locations of data coordinates #142

Merged
merged 23 commits into from
Jun 25, 2019

Conversation

takluyver
Copy link
Member

Discussion on #139

@takluyver takluyver mentioned this pull request Apr 4, 2019
karabo_data/geometry2.py Outdated Show resolved Hide resolved
karabo_data/geometry2.py Outdated Show resolved Hide resolved
karabo_data/geometry2.py Outdated Show resolved Hide resolved
@antarcticrainforest
Copy link
Contributor

LGTM, given that Henry is ok with it.

@takluyver
Copy link
Member Author

Thanks! Just to check: are you saying that you've spoken to Henry and he is happy with it, or that you're happy so long as it suits Henry?

@antarcticrainforest
Copy link
Contributor

Sorry, I meant LGTM from my side. No idea about Henry. I think he is on beamtime compensation or Holiday.

@takluyver
Copy link
Member Author

OK, thanks. When you see Henry back around, it might be good to remind him about this.

@takluyver takluyver changed the title WIP: Add get_pixel_positions() method Add get_pixel_positions() method Jun 11, 2019
@takluyver takluyver changed the title Add get_pixel_positions() method Add geometry methods to get physical locations of data coordinates Jun 11, 2019
@takluyver takluyver added this to the 0.5 milestone Jun 14, 2019
@fangohr
Copy link
Contributor

fangohr commented Jun 20, 2019

Do we have a notebook as an example that could go into the Karabo data documentation? I think this is interesting enough to be on read the docs. (Thinking in particular about the discussion with Andreas and the general need of having to know where pixels are for checking and integration/feeding of other tools etc).

@takluyver
Copy link
Member Author

I've just added a bit to the DSSC geometry notebook that was already there.

Copy link
Member

@tmichela tmichela left a comment

Choose a reason for hiding this comment

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

I'm trying to catch up with geometry code and I'm obviously not there yet...
I feel like it would be more efficient (for me) to wait until you're here next week if you have some time to waste on explaining this to me

karabo_data/geometry2.py Show resolved Hide resolved
for t, tile in enumerate(mod, start=0):
corner_x, corner_y, corner_z = tile.corner_pos * self.pixel_size
ss_unit_x, ss_unit_y, ss_unit_z = tile.ss_vec * self.pixel_size
fs_unit_x, fs_unit_y, fs_unit_z = tile.fs_vec * self.pixel_size
Copy link
Member

Choose a reason for hiding this comment

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

How do you define the corner position for a DSSC pixel, do you consider a box around the pixel?
I believe the positions in the slow scan axis will be wrong, due to the fact that the hexagon pixel is not regular and pixels are tessellated (considering the corner of a box around the pixel, it would be somewhere in an other pixel, e.g. corner of pixel [1, 0] would be in pixel [0, 0] - this is probably not clear, would be better if I could draw something...) and it looks like here that, e.g. pixel [1, 0] start at the edge of the pixel's [0, 0] box?

You probably got that right already and it's just me not getting the full picture... Maybe it would be easier if you could explain this to me next week?

Copy link
Member Author

Choose a reason for hiding this comment

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

I do consider a box around the pixel, so if you pass centre=False, it will give a point just outside the pixel (the corner of the bounding box). I'm OK with that. I'm planning to implement the distortion array method based on this, and that uses offsets from the corner of the bounding box.

However, I have made a mistake and got the centres slightly off. We should be adding 2/3 instead of 1/2 in the slow-scan direction - for the same reason as the 4/3 multiplication in the other PR you reviewed recently.

Copy link
Member Author

Choose a reason for hiding this comment

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

To illustrate what I meant about the centres being off, here's the plot before the correction:

pixel-centres-before2

And here it is after replacing the 1/2 shift with 2/3:

pixel-centres-after


Returns an array of similar shape with an extra dimension of length 3,
for (x, y, z) coordinates in metres.
"""
Copy link
Member

Choose a reason for hiding this comment

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

I do not understand this method and what should be the inputs :(

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll try to clarify it tomorrow. But briefly, it's for things like the output of the robust peak finder: it gives peak locations in pixel coordinates within the input data - e.g. 31.2 pixels in the slow-scan direction, 10.78 pixels fast scan. This method can convert those into coordinates in physical space (x, y, z).

@fangohr
Copy link
Contributor

fangohr commented Jun 21, 2019

Two more things:

Context: the plotting of the hexagons and the pixel positions is really great and useful in understanding and debugging things.

  1. Can you create yet another plot of this type that shows (parts of) (at least) two quadrants together? In particular, I want to emphasise the point that the relative x and y positions of these quadrants will only depend on the quadrant position parameters, and thus can be arbitrary.

I imagine this will also be useful when we evaluate various 'snapping' algorithms: we will then want to plot the 'assumed' pixel position on top of the real (hexagonal) pixel position.

  1. Can you extend this to also plot the 'assumed' positions for the plot_fast method? Ideally, we would draw the rectangular box of that pixel over the true hexagonal pixel. If we can see how this changes from one quadrant to the next, it would be perfect as a base for future development.

If it is too hard to squeeze in now, then please object and convert into a new ticket. I think we'll need it in any case.

@takluyver
Copy link
Member Author

  • Labelling axes: done
  • Reversed x: it's a consequence of the XFEL convention that we draw detector images looking along the beam, not into the beam.
  • Plotting centres and hexagons together: I've done this by adding an allow_negative_xy parameter to to_distortion_array, rather than showing the transformation in the notebook - I think it would be more confusing than useful.
  • Plotting multiple quadrants, rectangles over hexagons: I would recommend that we open new issues and tackle these separately. This PR is already pretty big, and I want to get a new release out soon. I'm also don't entirely follow what you want to show with parts of multiple quadrants.

@takluyver
Copy link
Member Author

The second new method is now called data_coords_to_positions(), and I've tried to document it better, including an example of using it in the AGIPD geometry notebook.

@codecov-io
Copy link

Codecov Report

Merging #142 into master will increase coverage by 0.37%.
The diff coverage is 97.5%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #142      +/-   ##
==========================================
+ Coverage   87.06%   87.44%   +0.37%     
==========================================
  Files          15       15              
  Lines        2450     2500      +50     
==========================================
+ Hits         2133     2186      +53     
+ Misses        317      314       -3
Impacted Files Coverage Δ
karabo_data/geometry2.py 94.03% <97.5%> (+1.01%) ⬆️

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 0cddc2d...404158c. Read the comment docs.

@takluyver
Copy link
Member Author

Got an offline LGTM from @tmichela

@takluyver takluyver merged commit fe7382e into master Jun 25, 2019
@takluyver takluyver deleted the geom-space-mapping branch June 25, 2019 11:42
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.

5 participants