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

Fix axis argument in call to sum() when scaling SI to normalize Poisson noise during lazy decomp #2111

Conversation

hakonanes
Copy link
Contributor

Progress of the PR

  • Change implemented,
  • add tests,
  • ready for review.

Minimal example of the bug fix

Running

>>> import hyperspy.api as hs
>>> s = hs.load(diffraction_patterns, lazy=True)
>>> s.change_dtype('float')
>>> s.decomposition(algorithm='PCA', output_dimension=200, normalize_poissonian_noise=True)

does no longer throw

Traceback (most recent call last):

  File "<ipython-input-1-88d7158fb358>", line 32, in <module>
    normalize_poissonian_noise=poisson_noise)

  File "/home/hakon/miniconda3/envs/pxm/lib/python3.6/site-packages/hyperspy/_signals/lazy.py", line 764, in decomposition
    data.sum(axis=range(ndim)),
[...]
  File "/home/hakon/miniconda3/envs/pxm/lib/python3.6/site-packages/dask/array/utils.py", line 144, in validate_axis
    raise TypeError("Axis value must be an integer, got %s" % axis)

TypeError: Axis value must be an integer, got range(0, 2)

since the it now reads tuple(range(ndim)) instead of just range(ndim).

@francisco-dlp
Copy link
Member

Looks good except that you should have branched it from Release next patch. Don't worry, I'll fix that, but next time think on branching from Rnp when you are submitting a bug fix.

Also, it'll be good to add a test. Would you like to do that? If not, don't worry, we can take care of it since, unfortunately, there are not tests for this feature that you can easily modify, what explains why this got broken without anybody noticing it.

@hakonanes
Copy link
Contributor Author

Oh, sorry, thanks for letting me know. I could write a test, however, I do not know where to start (tests/signal/test_lazy.py? tests/mva/test_decomposition.py?) since I have not done it before... so perhaps it is best if you do it.

@francisco-dlp
Copy link
Member

OK, I'll merge this and I'll open an issue about the missing test. Thanks for your contribuion!

@francisco-dlp francisco-dlp merged commit 5210c5d into hyperspy:RELEASE_next_minor Dec 7, 2018
@francisco-dlp francisco-dlp added this to the v1.4.2 milestone May 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants