Linear_Bin: function to bin spectrum by a non-integer number of pixels in each dimension. #1306
Conversation
Set version 2.0+dev
Linear_bin function added
Non Integer Binning
Revert "Non Integer Binning"
…Binning Revert "Revert "Non Integer Binning""
|
I would remove "Rebin Array" in the very first line of the docstring as well. It doesn't fit with the style of the rest of the docstrings. |
|
So the only thing failing now is:
E assert (2, 2, 1) == (1, 2, 6) |
|
Can't really look at it before the morning.
…On Wed, 24 May 2017 at 16:27, Katherine E. MacArthur < ***@***.***> wrote:
So the only thing failing now is:
def test_rebin(self):
self.signal.estimate_poissonian_noise_variance()
new_s = self.signal.rebin(scale=(2, 2, 1))
var = new_s.metadata.Signal.Noise_properties.variance
assert new_s.data.shape == (1, 2, 6)
assert var.data.shape == (1, 2, 6)
E assert (2, 2, 1) == (1, 2, 6)
E At index 0 diff: 2 != 1
E Use -v to get the full diff
but I'm not sure how the variance function works. Any ideas?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1306 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACmGj0kG7XA6nN1Jy-HR5g_n4yeUFxezks5r9D5rgaJpZM4KWsgD>
.
|
|
|
|
This is the line concerned https://github.com/hyperspy/hyperspy/blob/RELEASE_next_minor/hyperspy/signal.py#L2290 |
|
Ah, thanks, good spot. I missed that the rebin function calls itself... |
|
Why did you remove the lazy version? |
|
Because as @to266 pointed out, the function no longer runs lazily. Therefore I thought it might be cleaner to remove it completely so that there's no confusion later on if someone wants to add this back in. |
|
Wasn't the idea to use the "old" rebin when applicable and the linear one otherwise? In that case, |
|
Ok, try this then... I don't understand enough about the Lazy stuff but I've put it back in with the same if statements infront. Might need some testing. |
|
Tomorrow's a bank holiday weekend but I'll try and do everything I can for final tweaks on this function if you send them tonight. |
|
@k8macarthur, how is it going? Would you like a hand with this? |
|
Yes please, it compiles and all the tests pass, I think. It's just the lazy that needs testing now.
Best wishes,
Katherine MacArthur
… On 25 May 2017, at 12:25, Francisco de la Peña ***@***.***> wrote:
@k8macarthur, how is it going? Would you like a hand with this?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
| determined by the user. | ||
| Rebinning of the spectral data will be carried out by two methods. | ||
| The fast method will run if the new_shape is a divisor of the original shape. | ||
| Otherwise a slower linear interpolation method will be applied. Both methods are incorporated into the :py:meth:`~.signals.eds.rebin` function. |
There was a problem hiding this comment.
Not groundbreaking, but it should probably not point to specifically eds.rebin method.
| >>> spectrum.data[1, 2, 9] = 5 | ||
| >>> print(spectrum) | ||
| <EDSTEMSpectrum, title: , dimensions: (4, 4|10)> | ||
| >>> print ('Sum = ', sum(sum(sum(spectrum.data)))) |
There was a problem hiding this comment.
Sum over all elements like that: spectrum.data.sum()
| >>> test = spectrum.rebin(scale=scale) | ||
| >>> print(test) | ||
| <EDSTEMSpectrum, title: , dimensions: (8, 8|2)> | ||
| >>> print('Sum =', sum(sum(sum(test.data)))) |
| >>> spectrum.data[1, 2, 9] = 5 | ||
| >>> print(spectrum) | ||
| <EDSTEMSpectrum, title: , dimensions: (4, 4|10)> | ||
| >>> print ('Sum = ', sum(sum(sum(spectrum.data)))) |
| >>> test = spectrum.rebin(new_shape) | ||
| >>> print(test) | ||
| <EDSTEMSpectrum, title: , dimensions: (8, 8|2)> | ||
| >>> print('Sum =', sum(sum(sum(test.data)))) |
| #Series of if statements to check that only one out of new_shape or scale | ||
| #has been given. New_shape is then converted to scale. If both or neither | ||
| #are given the function raises and error and wont run. | ||
|
|
There was a problem hiding this comment.
Throughout the function, comparisons if something is None should be done using is operator, like so:
if new_shape is None and scale is not None:
# do stuff| Not both.") | ||
| elif new_shape != None: | ||
| scale = [] | ||
| for i, axis in enumerate(self.data.shape): |
There was a problem hiding this comment.
This does not check that new_shape length is appropriate.
I assume we require that full new shape is specified? I.e with signal shape (5, 3, 2) and new_shape=[2, 1] it fails?
Alternatively we could assume that the untouched shapes are kept constant, or any surplus shape is ignored.
In the first case (fails) the code should be:
#...
elif new_shape is not None:
if len(new_shape) != len(self.data.shape):
raise ValueError("Not enough dimensions in the new shape")
scale = [datashape/newshape for datashape, newshape in zip(self.data.shape, new_shape)]If we "pad" with ones and ignore the unnecessary, the code should be:
elif new_shape is not None:
lns = len(new_shape)
lds = len(self.data.shape)
if lns < lds:
new_shape = tuple(new_shape) + self.data.shape[lns:]
elif lns > lds:
new_shape = new_shape[:lds]
scale = [datashape/newshape for datashape, newshape in zip(self.data.shape, new_shape)]| scale.append(self.data.shape[i]/new_shape[i]) | ||
| else: | ||
| new_shape = new_shape | ||
| scale = scale |
There was a problem hiding this comment.
Actually this piece of code gets repeated at least 3-4 times, and thus should be moved to a separate function. I would move it to array_tools module or some place similar (i.e. not within a class at all), and its signature should be something along the lines of (feel free to use a better function name):
new_shape, scale = _rebin_preprocess(new_shape, scale, data_shape)Here the parameters should be whatever the user entered plus the actual current data shape, and the results should be what you'd have at this point in the function where this comment is. The new function should have the necessary corrections according to my previous comments
There was a problem hiding this comment.
I am working on this branch and I have addressed some of the code issues that you mention already. I'll push it soon, could you have a look? k8macarthur#12
|
Woohoo! Well done Kate and everyone :)
…On Fri, 26 May 2017 at 14:54, Francisco de la Peña ***@***.***> wrote:
Merged #1306 <#1306>.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1306 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ACmGj6tLk9LTxtmU5H6il6MBvqZ1_ITpks5r9tmkgaJpZM4KWsgD>
.
|
|
Excellent! Sorry I wasn't around for the final push. |
No description provided.