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 Signal and Model __call__ methods and remove Component.__call__ method. #3238

Merged
merged 13 commits into from Oct 28, 2023

Conversation

francisco-dlp
Copy link
Member

Description of the change

Rename the Model and Signal __call__ methods to _get_current_data and remove the unused Component.__call__ method.

While the previous syntax was compact, it was making the code hard to read for those who were not familiar with it.

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.

@francisco-dlp
Copy link
Member Author

Note that I have branched out from #3202. We must wait until #3202 is merged to merge this.

@codecov
Copy link

codecov bot commented Oct 10, 2023

Codecov Report

Attention: 10 lines in your changes are missing coverage. Please review.

Comparison is base (0df9467) 81.17% compared to head (0da946c) 80.98%.

Additional details and impacted files
@@                  Coverage Diff                   @@
##           RELEASE_next_major    #3238      +/-   ##
======================================================
- Coverage               81.17%   80.98%   -0.20%     
======================================================
  Files                     140      140              
  Lines                   20363    20359       -4     
  Branches                 4819     4819              
======================================================
- Hits                    16530    16488      -42     
- Misses                   2760     2808      +48     
+ Partials                 1073     1063      -10     
Files Coverage Δ
hyperspy/_components/gaussian.py 97.33% <100.00%> (ø)
hyperspy/_components/lorentzian.py 97.29% <100.00%> (ø)
hyperspy/_components/offset.py 94.33% <100.00%> (ø)
hyperspy/_components/polynomial.py 96.82% <ø> (ø)
hyperspy/_components/skew_normal.py 91.00% <100.00%> (ø)
hyperspy/_signals/complex_signal.py 93.75% <100.00%> (ø)
hyperspy/_signals/signal1d.py 75.09% <100.00%> (ø)
hyperspy/_signals/signal2d.py 88.39% <100.00%> (+6.54%) ⬆️
hyperspy/component.py 90.27% <100.00%> (-0.07%) ⬇️
hyperspy/drawing/markers.py 96.26% <100.00%> (ø)
... and 5 more

... and 2 files with indirect coverage changes

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

@ericpre
Copy link
Member

ericpre commented Oct 10, 2023

The alternative is to make the PR to the exspy branch just need to push the exspy branch to this repo (hyperspy/hyperspy) and change to that branch in this PR.
Anyway I m travelling for the rest of the week and forgot my laptop... 🤦‍♂️ So I will not do anything for the rest of the week.

@@ -0,0 +1,5 @@
Improve the readibility fo the code by replacing the ``__call__`` method of some objects with the more explicit ``_get_current_data``.
Copy link
Member

Choose a reason for hiding this comment

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

It may be worth mentioned explicitly that s() where s being an instance of BaseSignal is removed. Most users will not know that s() is the same as s.__call__.
As this privatise this functionality, it may be worth mentioned it explicitly here?

@ericpre ericpre mentioned this pull request Oct 27, 2023
12 tasks
@ericpre ericpre added this to the v2.0 Split milestone Oct 28, 2023
@ericpre ericpre merged commit ab8617f into hyperspy:RELEASE_next_major Oct 28, 2023
21 of 23 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

2 participants