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

Fixes for binned non_uniform_axes #2728

Merged
merged 20 commits into from May 10, 2021

Conversation

jlaehne
Copy link
Contributor

@jlaehne jlaehne commented May 2, 2021

Work on key issues from #2398

Description of the change

  • Fix plotting for binned spectrum images with non uniform axis obtained by convert_to_non_uniform_axis (index_in_array was lost)
  • Fix curve fitting for binned spectrum images with non-uniform axis
  • Handle occurences where 'scale' is used for binned signals also in the non-uniform case (e.g. in signals used for background removal)
  • Stacking along non-uniform axes
  • Raise warnings where still necessary, e.g. for EELS and EDX; add to docstrings
  • Test for warnings
  • Increase coverage of axes.py and other files affected by the non-uniform axis branch

Progress of the PR

  • fix plotting,
  • fix curve fitting,
  • handle 'scale' for non-uniform, binned signals,
  • stacking for non-uniform axes,
  • warnings for EELS and EDS models,
  • check for additional functions that need to raise warnings,
  • increase coverage,
  • add tests,
  • ready for review.

@jlaehne jlaehne mentioned this pull request May 2, 2021
39 tasks
@codecov
Copy link

codecov bot commented May 2, 2021

Codecov Report

Merging #2728 (967fe74) into non_uniform_axes (3ebb886) will increase coverage by 0.31%.
The diff coverage is 93.84%.

Impacted file tree graph

@@                 Coverage Diff                  @@
##           non_uniform_axes    #2728      +/-   ##
====================================================
+ Coverage             77.50%   77.81%   +0.31%     
====================================================
  Files                   203      203              
  Lines                 31066    31103      +37     
  Branches               6782     6791       +9     
====================================================
+ Hits                  24077    24204     +127     
+ Misses                 5159     5096      -63     
+ Partials               1830     1803      -27     
Impacted Files Coverage Δ
hyperspy/_signals/eds_sem.py 61.38% <ø> (+1.98%) ⬆️
hyperspy/_signals/hologram_image.py 88.61% <ø> (+3.96%) ⬆️
hyperspy/_signals/signal2d.py 80.47% <ø> (+1.20%) ⬆️
hyperspy/signal_tools.py 67.62% <0.00%> (-0.11%) ⬇️
hyperspy/_signals/eels.py 76.75% <66.66%> (+1.64%) ⬆️
hyperspy/models/model1d.py 87.96% <75.00%> (-0.34%) ⬇️
hyperspy/misc/utils.py 85.54% <90.90%> (+0.02%) ⬆️
hyperspy/_components/doniach.py 94.73% <100.00%> (+0.14%) ⬆️
hyperspy/_components/exponential.py 82.60% <100.00%> (+2.89%) ⬆️
hyperspy/_components/gaussian.py 97.33% <100.00%> (+2.59%) ⬆️
... and 26 more

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 3ebb886...967fe74. Read the comment docs.

@jlaehne
Copy link
Contributor Author

jlaehne commented May 5, 2021

The failing test in test_mrcz.py is unrelated (fails also in other branches).

For the remaining 3 Codecov warnings I am not sure which test to amend to catch these lines.

@thomasaarholt
Copy link
Contributor

Aside from a few small comments, LGTM! Excellent choice of axis name and units in the tests - "lovely plumage" :)

@jlaehne
Copy link
Contributor Author

jlaehne commented May 6, 2021

Aside from a few small comments, LGTM! Excellent choice of axis name and units in the tests - "lovely plumage" :)

Yes, we definitely need more Monty (in HyperSpy's) Python :)

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.

A first iteration of comments, I may have some more later.

hyperspy/_components/doniach.py Outdated Show resolved Hide resolved
hyperspy/_signals/signal1d.py Show resolved Hide resolved
hyperspy/tests/signals/test_remove_background.py Outdated Show resolved Hide resolved
hyperspy/axes.py Show resolved Hide resolved
hyperspy/signal.py Outdated Show resolved Hide resolved
hyperspy/tests/model/test_model.py Show resolved Hide resolved
hyperspy/_components/doniach.py Outdated Show resolved Hide resolved
hyperspy/tests/model/test_model.py Show resolved Hide resolved
hyperspy/_signals/eds_tem.py Show resolved Hide resolved
hyperspy/axes.py Show resolved Hide resolved
hyperspy/misc/utils.py Show resolved Hide resolved
hyperspy/misc/utils.py Outdated Show resolved Hide resolved
hyperspy/misc/utils.py Outdated Show resolved Hide resolved
hyperspy/misc/utils.py Outdated Show resolved Hide resolved
hyperspy/signal.py Outdated Show resolved Hide resolved
hyperspy/_components/exponential.py Outdated Show resolved Hide resolved
hyperspy/signal.py Outdated Show resolved Hide resolved
hyperspy/signal.py Outdated Show resolved Hide resolved
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.

Looks good to me. There is one docstring left to update and two comments I will leave it to you, if you want to take them into account or not.

@jlaehne
Copy link
Contributor Author

jlaehne commented May 9, 2021

I took into account all the comments, thanks for all the feedback @ericpre and @thomasaarholt.

@thomasaarholt thomasaarholt self-requested a review May 10, 2021 07:37
@thomasaarholt thomasaarholt merged commit a3b7b87 into hyperspy:non_uniform_axes May 10, 2021
@thomasaarholt
Copy link
Contributor

Really nice @jlaehne!

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