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 context manager to switch iterpath #2795

Merged
merged 6 commits into from Jul 28, 2021

Conversation

ericpre
Copy link
Member

@ericpre ericpre commented Jul 8, 2021

Progress of the PR

  • Add context manager to set switch iterpath,
  • Remove _iterate_signal of lazy signal, its non-lazy counterpart works fine
  • simplify _iterate_signal and make it use __call__ so that lazy signal would benefit from the speed improvement in Improve plotting of lazy signals, by keeping current chunk #2568,
  • update docstring (if appropriate),
  • update user guide (if appropriate),
  • Fix warning when building user guide,
  • add an changelog entry in the upcoming_changes folder (see upcoming_changes/README.rst),
  • Check formatting changelog entry in the readthedocs doc build of this PR (link in github checks)
  • add tests,
  • ready for review.

Minimal example of the bug fix or the new feature

import hyperspy.api as hs
import numpy as np

s = hs.signals.Signal1D(np.arange(2*3*4).reshape([3, 2, 4]))

with s.axes_manager.switch_iterpath('serpentine'):
    for indices in s.axes_manager:
        print(indices)

@ericpre ericpre added this to the v1.7 milestone Jul 8, 2021
@codecov
Copy link

codecov bot commented Jul 8, 2021

Codecov Report

Merging #2795 (2bb9976) into RELEASE_next_minor (1eb0bdf) will decrease coverage by 0.00%.
The diff coverage is 96.29%.

Impacted file tree graph

@@                  Coverage Diff                   @@
##           RELEASE_next_minor    #2795      +/-   ##
======================================================
- Coverage               78.44%   78.43%   -0.01%     
======================================================
  Files                     203      203              
  Lines                   31269    31253      -16     
  Branches                 6861     6857       -4     
======================================================
- Hits                    24528    24514      -14     
+ Misses                   4980     4978       -2     
  Partials                 1761     1761              
Impacted Files Coverage Δ
hyperspy/_signals/lazy.py 91.65% <ø> (+0.29%) ⬆️
hyperspy/datasets/artificial_data.py 98.64% <ø> (ø)
hyperspy/signal.py 75.92% <90.00%> (-0.20%) ⬇️
hyperspy/_signals/signal1d.py 77.77% <100.00%> (+0.04%) ⬆️
hyperspy/axes.py 90.59% <100.00%> (+0.08%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1eb0bdf...2bb9976. Read the comment docs.

Copy link
Contributor

@thomasaarholt thomasaarholt left a comment

Choose a reason for hiding this comment

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

Lots of nice things here!

LGTM!

"""Iterates over the signal data.
def _iterate_signal(self, iterpath=None):
"""Iterates over the signal data. It is faster than using the signal
iterator.
Copy link
Contributor

Choose a reason for hiding this comment

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

What is "the signal iterator" that this is faster than? Assuming you mean for si in s?
How about """Iterates over the signal data. It is faster than iterating over a signal, since it returns the array directly and not a HyperSpy signal.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, it would be useful to explain why it is faster! I don't remember right now and I will need to look it up again.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that was my best guess :p

Copy link
Member Author

Choose a reason for hiding this comment

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

What is "the signal iterator" that this is faster than?

The BaseSignal class has an iterator implementation, i. e. __iter__ and __next__ are implemented, see https://docs.python.org/3/library/stdtypes.html#iterator-types for more explanation.

@ericpre
Copy link
Member Author

ericpre commented Jul 26, 2021

@thomasaarholt, I have improved the docstring, can you please have a look at it again?

@thomasaarholt
Copy link
Contributor

Yep, that looks good and is sensible! You can merge when you want :)

@ericpre ericpre merged commit e7a0267 into hyperspy:RELEASE_next_minor Jul 28, 2021
@ericpre ericpre deleted the fix_iterate_signal branch July 28, 2021 19:53
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

2 participants