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

Azimuthal integral off-by-one bugfix #1058

Merged
merged 15 commits into from Apr 17, 2024

Conversation

viljarjf
Copy link
Contributor

@viljarjf viljarjf commented Apr 9, 2024


Fixes an off-by-one error in azimuthal integration

Checklist

  • implementation steps
  • update the Changelog
  • mark as ready for review

What does this PR do? Please describe and/or link to an open issue.
An off-by-one error meant the final row and column of the signal was ignored when transforming to polar coordinates with Diffraction2D.get_azimuthal_integral2d.
In the old code, the new test fails for all corner != (0, 0), i.e. all corners at the last row and/or column.
The new implementation calculates the extent in cartesian space of each pixel, accounting for the curvature of Ewald's sphere where applicable, and uses those for utils._azimuthal_integrations._get_factors.

The previous implementation relied on the Calibration.axes, where the final element was not included due to the off-by-one error. Furthermore, from looking at hyperspy plots with few signal pixels, the coordinates of the axes represent the centers rather than the edges of the pixels, which is now accounted for

@viljarjf
Copy link
Contributor Author

viljarjf commented Apr 9, 2024

It seems the docs failed because I assumed a UniformDataAxis, i.e. a flat ewald's sphere.
If the distance between pixels is not uniform, then I'm not entirely sure how to account for the width of the final row/column.
I believe I would have to replicate part of the code in the Calibrate.detector responsible for setting the axis.
Any thoughts, @CSSFrancis?

@coveralls
Copy link

coveralls commented Apr 9, 2024

Coverage Status

coverage: 92.941% (-0.02%) from 92.965%
when pulling a4c87f7 on viljarjf:azimuthal-integral-off-by-one-bugfix
into b88a993 on pyxem:main.

@pc494 pc494 requested a review from CSSFrancis April 9, 2024 17:50
@viljarjf
Copy link
Contributor Author

viljarjf commented Apr 9, 2024

Non-flat Ewald's sphere seems to work now, but apart from manually inspecting some plots with exaggerated pixel sizes and camera lengths I don't know how to test it...

The implementation now calculates the extent in cartesian space for each pixel, accounting for the curvature of Ewald's sphere where applicable.

@viljarjf
Copy link
Contributor Author

viljarjf commented Apr 9, 2024

Marking as draft, some unrelated tests fail now and I don't have more time today

@viljarjf viljarjf marked this pull request as draft April 9, 2024 18:45
@CSSFrancis
Copy link
Member

Marking as draft, some unrelated tests fail now and I don't have more time today

@viljarjf Do you want me to look at this later? I can help with the failing tests and review.

@viljarjf
Copy link
Contributor Author

viljarjf commented Apr 9, 2024

@CSSFrancis If you want, sure! I believe the tests fail because I changed the way the center index was calculated in Calibration.detector, it might need to align with ElectronDiffraction2D.set_diffraction_calibration

@viljarjf
Copy link
Contributor Author

It was the center coordinates. The pixel coordinate of np.arange(10) is at 4.5, which is rounded down to 4. This is different from the previous implementation, which used ax.size / 2, e.g. 5 instead of 4.5.

@viljarjf
Copy link
Contributor Author

I also want to add support for the azimuthal_range-argument, which currently only affects the axis rather than the actual integration.
It would probably be nice for some usecases if you could specify to only integrate from 0 to pi, for example.
This would also ensure the polar unwrapping has a consisteltly defined 0 in the axes coordinates, which would be useful in #1018 to ensure the polar signal is aligned consistently.

I have the code ready, but it might fit better in another PR? Alternatively, I could change the subject of this PR to be more broad.

@viljarjf viljarjf marked this pull request as ready for review April 10, 2024 09:49
Copy link
Member

@CSSFrancis CSSFrancis left a comment

Choose a reason for hiding this comment

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

@viljarjf This is good, thanks for cleaning up my mistakes :)

We should also make an example showing how pixels are defined in pyxem. In this case we use the center of the pixel to determine the coordinates rather than the upper right hand corner?

pyxem/utils/calibration.py Outdated Show resolved Hide resolved
@CSSFrancis
Copy link
Member

@viljarjf I think this is good! Any chance you can look over the example I just added regarding the coordinate conventions.

@CSSFrancis
Copy link
Member

I also want to add support for the azimuthal_range-argument, which currently only affects the axis rather than the actual integration. It would probably be nice for some usecases if you could specify to only integrate from 0 to pi, for example. This would also ensure the polar unwrapping has a consisteltly defined 0 in the axes coordinates, which would be useful in #1018 to ensure the polar signal is aligned consistently.

I have the code ready, but it might fit better in another PR? Alternatively, I could change the subject of this PR to be more broad.

Let's add that as a separate PR as this one seems good to go. If only to let you run the CI without approval :)

@CSSFrancis
Copy link
Member

@viljarjf Do you want to update the changelog as well?

@CSSFrancis CSSFrancis merged commit 5c6a458 into pyxem:main Apr 17, 2024
15 of 16 checks passed
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.

None yet

3 participants