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

Crop edges update simplified #177

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

jordiferrero
Copy link
Contributor

@jordiferrero jordiferrero commented Jan 29, 2023

Description of the change

Following the request to make #142 cleaner and separate the crop_ede function from other changes, this is the clean version of the PR, aiming to extend the functionality of crop_edges.

Progress of the PR

  • Change implemented (can be split into several points),
  • docstring updated (if appropriate),
  • update user guide (if appropriate),
  • added tests,
  • added line to CHANGELOG.md,
  • ready for review.

@codecov-commenter
Copy link

codecov-commenter commented Jan 29, 2023

Codecov Report

Base: 100.00% // Head: 100.00% // No change to project coverage 👍

Coverage data is based on head (8d66281) compared to base (cc5507b).
Patch coverage: 100.00% of modified lines in pull request are covered.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #177   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           12        12           
  Lines          545       614   +69     
=========================================
+ Hits           545       614   +69     
Impacted Files Coverage Δ
lumispy/signals/common_luminescence.py 100.00% <100.00%> (ø)
lumispy/utils/signals.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

lumispy/utils/signals.py Fixed Show fixed Hide fixed
@jordiferrero
Copy link
Contributor Author

As discussed, this is now ready for review @jlaehne . Hope it is easier to see what is new.

@@ -23,47 +23,18 @@

from numpy import isnan
from warnings import warn
from lumispy.utils.signals import crop_edges


class CommonLumi:
"""**General luminescence signal class (dimensionless)**"""

def crop_edges(self, crop_px):
Copy link
Contributor

Choose a reason for hiding this comment

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

To make our work for the 1.0 release easier, add a comment in the line before:
# Deprecated, to be removed for v1.0 release

@@ -23,18 +23,12 @@


class TestCommonLumi:
def test_crop_edges(self):
def test_crop_edges_deprecated(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

add comment before # Deprecated, to be removed for v1.0 release

>>> signals_cropped = lum.utils.crop_edges(signals, crop_range=5, crop_units="%", rebin_nav=True)
>>> signals_cropped
[CLSpectrum <243,243|1024>, Signal2D <243,243|1>]
.. Note::
Copy link
Contributor

Choose a reason for hiding this comment

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

Add blank line before

Cropping along the navigation axes of a list of signal objects.
Crop the amount of pixels from the four edges of the scanning
region, from the edges inwards. Cropping can happen uniformly on all
sides or by specifying the cropping range for each axis or each side. If the navigation axes shape is different, all signals can be rebinned to match the shape of the first signal in the list.
Copy link
Contributor

Choose a reason for hiding this comment

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

Continue with line breaks for readability across the whole docstring and also in error messages below.

sides or by specifying the cropping range for each axis or each side. If the navigation axes shape is different, all signals can be rebinned to match the shape of the first signal in the list.
Parameters
----------
S : list of HyperSpy Signal objects or a single HyperSpy Signal object.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
S : list of HyperSpy Signal objects or a single HyperSpy Signal object.
S : Signal or list of Signals
HyperSpy signal object(s) that should be cropped.

If the navigation axes shape is different between signals in the list S, all signals will be rebinned to match the shape of the first signal in the list. Note this does not take into account the calibration values of the navigation axes.
kwrgs
To account for the deprecated ``crop_px`` parameter.
Returns
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Returns
Returns

@jlaehne jlaehne added this to the v0.2.2 milestone Feb 16, 2023
@jlaehne jlaehne linked an issue Feb 16, 2023 that may be closed by this pull request
@jlaehne jlaehne mentioned this pull request Mar 13, 2023
6 tasks
@jlaehne jlaehne modified the milestones: v0.2.2, v0.2.3 Mar 15, 2023
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.

Extend functionality of crop_edges
3 participants