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 option to set eta_step size for eta omega maps #573

Merged
merged 5 commits into from
Dec 4, 2023

Conversation

psavery
Copy link
Collaborator

@psavery psavery commented Oct 27, 2023

It was previously fixed to 0.25 degrees. However, the users may find it beneficial to change this.

Smaller eta_step sizes might lead to less overlap between neighboring peaks in the eta omega maps, for example.

@psavery psavery marked this pull request as draft October 27, 2023 21:38
@psavery
Copy link
Collaborator Author

psavery commented Oct 27, 2023

Although this works, I found an issue with modifying eta_step. Detectors that have a split in eta (i. e., contain -180 next to 180) don't appear correct for smaller eta_step values, such as 0.05. I believe the issue is here

The problem is that for a detector split in eta, we need to remap the angles to be increasing with no gap. So, for instance, for dexelas, ff2 needs to go from 98.5 degrees to 261.5 degrees.

Something in the logic in that function is breaking for small eta_step so that the eta values don't get ordered correctly, and that's making the histogram look wrong. We need to fix that issue.

@psavery
Copy link
Collaborator Author

psavery commented Nov 13, 2023

@donald-e-boyce This is working now.

@psavery
Copy link
Collaborator Author

psavery commented Nov 14, 2023

For reference, here are eta omega maps produced using different eta steps. A smaller eta step produces a higher resolution along the eta axis. The default eta value is 0.25, and that was hardcoded before this PR.

1.0

1 00_eta

0.25

0 25_eta

0.05

0 05_eta

0.01

0 01_eta

@psavery psavery requested a review from bnmajor November 14, 2023 17:51
@psavery
Copy link
Collaborator Author

psavery commented Nov 29, 2023

@donald-e-boyce I made your suggested changes and removed the histogramming entirely. It seems to work well! This is ready for your review.

@psavery
Copy link
Collaborator Author

psavery commented Nov 29, 2023

Unfortunately, removing the histogramming appears to have changed two things:

  1. The places where eta doesn't lie on a detector used to be np.nan, but not after these changes (it is 0 instead)
  2. The actual data appears different. The new data has fewer spots. I don't immediately know why.

psavery and others added 4 commits December 4, 2023 10:39
It was previously fixed to 0.25 degrees. However, the users may find
it beneficial to change this.

Smaller eta_step sizes might lead to less overlap between neighboring
peaks in the eta omega maps, for example.

Signed-off-by: Patrick Avery <patrick.avery@kitware.com>
Smaller values in eta step were breaking the branch cut logic here.

The problem is that the arc length was not always less than 1e-4 when
the eta_step was small (such as 0.05). It was less than 1e-2, so still
close to zero, but not small enough.

Then on top of that, some of the ring break logic was not working.

To fix both of those issues, we loosen the logic to instead check to
see if nearly the whole two pi range is covered, and if there is a big
gap. If there is, then we assume it is a branch cut. And we locate the
break via the big gap.

This worked properly for eta step values from 0.01 to 1

Signed-off-by: Patrick Avery <patrick.avery@kitware.com>
This change was suggested by @donald-e-boyce

He mentioned that the whole histogramming should be unnecessary, and
we should just simplify the code to not use it.

This appears to work properly for the different `eta_step` sizes as
before. Thanks Don!

Signed-off-by: Patrick Avery <patrick.avery@kitware.com>
This is easier than the logic checks we were doing before.

Signed-off-by: Patrick Avery <patrick.avery@kitware.com>
@psavery
Copy link
Collaborator Author

psavery commented Dec 4, 2023

@donald-e-boyce helped me fix this.

The only potential issue is that, for a small eta step, there are some nans on the detector plates. However, indexing still worked fine. We will explore fixing this in the future, if needed.

@psavery psavery merged commit 3372ec3 into master Dec 4, 2023
6 checks passed
@psavery psavery deleted the eta-ome-maps-eta-step branch December 4, 2023 17:36
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.

2 participants