-
Notifications
You must be signed in to change notification settings - Fork 85
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
Fit linear plane in pyxem.signals.BeamShift.get_linear_plane by minimising magnitude variance #1116
base: main
Are you sure you want to change the base?
Conversation
@sivborg This PR looks good! Would you like to add something to the "Centering the Zero Beam" example or would you like me to add that? I can help with creating a test dataset is that helps. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple of basic comments.
pyxem/signals/beam_shift.py
Outdated
[d/dx, d/dy, x_0, d/dx, d/dy, y_0] | ||
where the first three entries are for the x-shift, being in order the | ||
step in x, step in y and the initial value at (0, 0). Similarly for the | ||
last three entries for the y-shift. Currently only implemented for the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you maybe reword this? I'm not exactly sure what these parameters are/ what they do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, these parameters are a bit hard to briefly introduce, especially as we have to differentiate between an x-shift and an x-position. I will try to reword it.
pyxem/signals/beam_shift.py
Outdated
mask=None, | ||
fit_corners=None, | ||
initial_values=None, | ||
constrain_magnitude=False, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason for constrain_magnitude
to be False instead of True by default? It seems like this would work quite well in most cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
constrain_magnitude
is a bit imprecise, as I reread it. To be precise, it actually minimises the magnitude variance. So I think I will rename it constrain_magnitude_variance
to be more descriptive.
And I think it will not work well by default as it does not pay attention to the actual value of the magnitude, only its variance. When fitting a linear plane to a non-electromagnetic area, which is the most common use case, you actually want to minimise the magnitude itself, which it would not try to do if constrain_magnitude_variance
is True
. Maybe there would be a good way to minimise both the magnitude and variance at the same time, with a better cost function or some such, but it does not seem obvious for me.
Thank you for looking over! I can quickly expand the example, although admittedly this is a quite niche use case as it is only necessary if (a) your entire sample is electromagnetic and (b) there are several domains of equal field strength which you can fit your plane in. But I think it might be useful for people to try here and there, so I can make it more visible through the example! |
@CSSFrancis I have added the new functionality to the beam centering example, although creating the example dataset is a bit ugly and should probably be encapsulated somewhere. So please add or change anything that is needed. |
Fit linear plane in
pyxem.signals.BeamShift.get_linear_plane
by minimising magnitude varianceChecklist
What does this PR do? Please describe and/or link to an open issue.
In my work, there can be electromagnetic field in the sample areas where I want to fit linear planes for descan removal. However, using the existing algorithm in
pyxem.signals.BeamShift.get_linear_plane
by minimising the magnitudes through least squares gives poor results.What is better for my case is assuming the sample has uniform electromagnetic field strengths and therefore uniform deflection magnitudes, then fitting the linear planes from this. This is done by minimising the deflection magnitude variance so that ther is a constant deflection magnitude after fitting. This can be used by setting
constrain_magnitude
toTrue
:However there are a couple issues. If there are few domains, then there will be many valid planes for this fitting, some very nonsensical. Therefore, I have exposed the
initial_values
parameter, which you can vary to get different results. Part of the reason is that since we fit the planes together, we have six parameters and therefore the algorithm seems to get stuck in many local minima. See the tests for an example where this is used to fix things. The algorithm is also very affected by noise, so a mask is often necessary where this is severe.I have also added a check for the
mask
having a datatype of booleans. It can accept other datatypes silently, where it would most likely give wildly wrong results. So I found it useful to have an explicit check.Regardless this is a very useful feature for me that has helped with my data-analysis. So I hope we can share it with others!