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

Rename crop image #3197

Merged
merged 5 commits into from Nov 2, 2023
Merged

Conversation

CSSFrancis
Copy link
Member

@CSSFrancis CSSFrancis commented Jul 19, 2023

Description of the change

Renamed Signal2D.crop_image to Signal2D.crop_signal2D

Progress of the PR

  • Change implemented (can be split into several points),
  • update docstring (if appropriate),
  • update user guide (if appropriate),
  • 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.

@CSSFrancis CSSFrancis changed the base branch from RELEASE_next_minor to RELEASE_next_major July 19, 2023 16:58
@CSSFrancis CSSFrancis mentioned this pull request Jul 19, 2023
57 tasks
@codecov
Copy link

codecov bot commented Jul 19, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9ecef39) 80.84% compared to head (bd2b091) 80.79%.

Additional details and impacted files
@@                  Coverage Diff                   @@
##           RELEASE_next_major    #3197      +/-   ##
======================================================
- Coverage               80.84%   80.79%   -0.05%     
======================================================
  Files                     140      140              
  Lines                   20361    20361              
  Branches                 4821     4821              
======================================================
- Hits                    16460    16450      -10     
- Misses                   2827     2838      +11     
+ Partials                 1074     1073       -1     
Files Coverage Δ
hyperspy/_signals/signal1d.py 75.09% <100.00%> (ø)
hyperspy/_signals/signal2d.py 88.39% <100.00%> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jlaehne
Copy link
Contributor

jlaehne commented Aug 7, 2023

This renaming looks plausible to me, but as it breaks the API, it would be great to get a few more opinions on this.

@jlaehne
Copy link
Contributor

jlaehne commented Aug 7, 2023

The code-block example in the user guide will need to be corrected, currently it reads
im.crop(...)

@ericpre
Copy link
Member

ericpre commented Sep 28, 2023

Initially wanted to rebase and update the user guide and I realised that it could simplify to crop_signal for Signal1D and Signal2D. What do you think?

@ericpre
Copy link
Member

ericpre commented Sep 28, 2023

@CSSFrancis, apparently I can't request a review from you on your own pull request! 😅

doc/user_guide/signal1d.rst Outdated Show resolved Hide resolved
doc/user_guide/signal1d.rst Outdated Show resolved Hide resolved
doc/user_guide/signal1d.rst Outdated Show resolved Hide resolved
@ericpre
Copy link
Member

ericpre commented Oct 2, 2023

This conflict with #3202, will come back to it once #3202 is merged.

@ericpre ericpre merged commit 70f3494 into hyperspy:RELEASE_next_major Nov 2, 2023
20 of 22 checks passed
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