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

Fix estimate_parameters for binned and non uniform axis #2743

Merged

Conversation

LMSC-NTappy
Copy link
Contributor

@LMSC-NTappy LMSC-NTappy commented May 16, 2021

Description of the change

Rebase of PR #2740 with the correct version of non_uniform_axes.
Fixes #2739.

Summary: this PR fixes incorrect behaviour in BaseDataAxis.value2index() which was not accepting arrays as input and was computing the axis gradient even for non-binned axes.

Progress of the PR

  • Change implemented (can be split into several points),
  • update docstring (if appropriate),
  • add tests,
  • ready for review.

@codecov
Copy link

codecov bot commented May 16, 2021

Codecov Report

Merging #2743 (9574ada) into non_uniform_axes (a737190) will decrease coverage by 0.13%.
The diff coverage is 81.30%.

Impacted file tree graph

@@                 Coverage Diff                  @@
##           non_uniform_axes    #2743      +/-   ##
====================================================
- Coverage             78.02%   77.88%   -0.14%     
====================================================
  Files                   203      203              
  Lines                 31607    31144     -463     
  Branches               7020     6807     -213     
====================================================
- Hits                  24662    24258     -404     
+ Misses                 5130     5086      -44     
+ Partials               1815     1800      -15     
Impacted Files Coverage Δ
hyperspy/_components/volume_plasmon_drude.py 47.05% <ø> (ø)
hyperspy/misc/array_tools.py 76.35% <30.00%> (-7.25%) ⬇️
hyperspy/axes.py 90.10% <62.50%> (+0.10%) ⬆️
hyperspy/_components/arctan.py 66.66% <100.00%> (ø)
hyperspy/_components/bleasdale.py 100.00% <100.00%> (ø)
hyperspy/_components/doniach.py 94.87% <100.00%> (+0.13%) ⬆️
hyperspy/_components/eels_arctan.py 89.47% <100.00%> (ø)
hyperspy/_components/eels_double_power_law.py 96.42% <100.00%> (ø)
hyperspy/_components/error_function.py 81.81% <100.00%> (ø)
hyperspy/_components/exponential.py 82.60% <100.00%> (ø)
... and 27 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 a737190...9574ada. Read the comment docs.

hyperspy/axes.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.

This looks good to me and I have left a couple of comments.
I will leave someone else to review/merge it since I influenced this PR!

hyperspy/tests/axes/test_data_axis.py Show resolved Hide resolved
hyperspy/component.py Outdated Show resolved Hide resolved
hyperspy/axes.py Show resolved Hide resolved
hyperspy/axes.py Outdated Show resolved Hide resolved
hyperspy/tests/axes/test_data_axis.py Outdated Show resolved Hide resolved
@ericpre ericpre changed the title Fixes for Fix estimate_parameters for binned and non uniform axis May 16, 2021
@jlaehne
Copy link
Contributor

jlaehne commented May 17, 2021

Looks good on first sight, I'll have a closer look in the next days when I find some time.

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.

This looks good to me, just need a tiny bit of tidying up. @jlaehne, I leave it to you. Thanks!

Copy link
Contributor

@jlaehne jlaehne left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the help with the nua branch! I left a few questions, but as said previously this looks good otherwise.

hyperspy/_components/exponential.py Show resolved Hide resolved
hyperspy/signal.py Show resolved Hide resolved
hyperspy/signal.py Outdated Show resolved Hide resolved
hyperspy/tests/axes/test_data_axis.py Show resolved Hide resolved
hyperspy/tests/axes/test_data_axis.py Show resolved Hide resolved
hyperspy/tests/axes/test_data_axis.py Show resolved Hide resolved
hyperspy/axes.py Show resolved Hide resolved
hyperspy/axes.py Show resolved Hide resolved
@jlaehne
Copy link
Contributor

jlaehne commented May 27, 2021

OK, I merge this PR as the main objectives are more than covered. Further optimization that we discussed at several points can be contributed as new PRs. Thanks again!

@jlaehne jlaehne merged commit 516b193 into hyperspy:non_uniform_axes May 27, 2021
@LMSC-NTappy
Copy link
Contributor Author

This was a nice ride. Thank you!

@LMSC-NTappy LMSC-NTappy deleted the fixes_binned_nua_2_rebase branch May 28, 2021 06:06
@ericpre ericpre mentioned this pull request May 31, 2021
39 tasks
@jlaehne jlaehne mentioned this pull request Jun 7, 2021
5 tasks
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.

nan and maps non supported by current implementation of scaling factor for binned non-uniform axes
3 participants