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

plot residual #3186

Merged
merged 5 commits into from Aug 26, 2023
Merged

plot residual #3186

merged 5 commits into from Aug 26, 2023

Conversation

HanHsuanWu
Copy link
Contributor

@HanHsuanWu HanHsuanWu commented Jun 27, 2023

Description of the change

A few sentences and/or a bulleted list to describe and motivate the change:

As suggested by @thomasaarholt on gitter, I added a plot_residual argument to model1d.plot.
This argument will plot a residual signal (Signal - Model) to the signal plot.
Currently, it has a bug when the size of signal and model are not the same. I am not sure what's the best way to do this so any help is appreciated.

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.

Minimal example of the bug fix or the new feature

import hyperspy.api as hs
import numpy as np
s = hs.signals.Signal1D(np.tile(np.arctan(np.arange(-500, 500)), (10, 1)),dtype='float64')
s.add_gaussian_noise(std=0.1)
m = s.create_model()
line  = hs.model.components1D.Polynomial(order=2)
m.append(line)
m.fit()
m.plot(plot_residual=True)

@codecov
Copy link

codecov bot commented Jun 27, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.01% 🎉

Comparison is base (aeae846) 81.25% compared to head (18a019a) 81.27%.

Additional details and impacted files
@@                  Coverage Diff                   @@
##           RELEASE_next_major    #3186      +/-   ##
======================================================
+ Coverage               81.25%   81.27%   +0.01%     
======================================================
  Files                     173      173              
  Lines                   24192    24203      +11     
  Branches                 5621     5622       +1     
======================================================
+ Hits                    19657    19670      +13     
+ Misses                   3236     3234       -2     
  Partials                 1299     1299              
Files Changed Coverage Δ
hyperspy/models/model1d.py 88.67% <100.00%> (+0.27%) ⬆️

... and 1 file with indirect coverage changes

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

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.

Thanks for the PR! I've added some general linting comments below as well as a few suggestions.


def _residual2plot(self, axes_manager, out_of_range2nans=True):
old_axes_manager = None
if axes_manager is not self.axes_manager:
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you document what happens here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added documentation for _residual_for_plot.

hyperspy/models/model1d.py Outdated Show resolved Hide resolved
hyperspy/models/model1d.py Outdated Show resolved Hide resolved
hyperspy/models/model1d.py Outdated Show resolved Hide resolved
@HanHsuanWu
Copy link
Contributor Author

Hey @thomasaarholt thanks for reviewing.
I have addressed your comments and ended up deleting the out_of_range2nans part.
I added a new argument for model1d.call() to ignore the channel_switches, so that it always plots the residual for the whole spectrum. We can add another argument to turn this on or off if needed. What do you think?

Copy link
Member

@ericpre ericpre left a comment

Choose a reason for hiding this comment

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

Thank you @HanHsuanWu, this works well and is a long overdue!

I have suggested some simplification.

This would need a changelog entry.

hyperspy/models/model1d.py Outdated Show resolved Hide resolved
hyperspy/models/model1d.py Outdated Show resolved Hide resolved
@HanHsuanWu
Copy link
Contributor Author

Thank you @HanHsuanWu, this works well and is a long overdue!

I have suggested some simplification.

This would need a changelog entry.

What is a changelog entry and where do I add this?

@ericpre
Copy link
Member

ericpre commented Jul 4, 2023

See https://github.com/hyperspy/hyperspy/blob/RELEASE_next_minor/upcoming_changes/README.rst as per checklist of the first comment of the PR.
It is a note to describe the changes of this PR and will be added automatically to the list of changes (also called changelog) for each release.

@jlaehne jlaehne mentioned this pull request Aug 7, 2023
7 tasks
@HanHsuanWu
Copy link
Contributor Author

See https://github.com/hyperspy/hyperspy/blob/RELEASE_next_minor/upcoming_changes/README.rst as per checklist of the first comment of the PR. It is a note to describe the changes of this PR and will be added automatically to the list of changes (also called changelog) for each release.

Thanks Eric. I have added the changelog. Got a bit confused before.

@HanHsuanWu
Copy link
Contributor Author

@ericpre any further comments for this pull request?

@ericpre
Copy link
Member

ericpre commented Aug 25, 2023

Sorry, I don't have time at the minute, I will come back to it very soon.

@ericpre ericpre changed the base branch from RELEASE_next_minor to RELEASE_next_major August 26, 2023 15:34
Copy link
Member

@ericpre ericpre left a comment

Choose a reason for hiding this comment

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

Thanks @HanHsuanWu, this looks good.
In 18a019a I added a test and an example. I have also rebased on the RELEASE_next_major branch as the next release will be the 2.0.

@ericpre ericpre merged commit 803679a into hyperspy:RELEASE_next_major Aug 26, 2023
23 checks passed
@jlaehne jlaehne added this to the v2.0 Split milestone Aug 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants