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

Add support for "sig" and "nav" to index axes_manager #3385

Merged

Conversation

ericpre
Copy link
Member

@ericpre ericpre commented Jun 6, 2024

This is a simple change but very convenient!

Progress of the PR

  • Add support for "sig" and "nav" to index axes_manager,
  • update docstring (if appropriate),
  • update user guide (I may have another to see where best to update),
  • 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.ones((10, 10, 10)))

assert s.axes_manager["nav"] == s.axes_manager.navigation_axes
assert s.axes_manager["sig"] == s.axes_manager.signal_axes

# Sum over navigation axis
s_sum_nav = s.sum(axis="nav")

# Sum over signal axis
s_sum_sig = s.sum(axis="sig")

Copy link

codecov bot commented Jun 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.65%. Comparing base (70b9669) to head (daba5e9).

Additional details and impacted files
@@                  Coverage Diff                   @@
##           RELEASE_next_minor    #3385      +/-   ##
======================================================
+ Coverage               80.63%   80.65%   +0.01%     
======================================================
  Files                     146      146              
  Lines                   21918    21929      +11     
  Branches                 5174     5178       +4     
======================================================
+ Hits                    17674    17687      +13     
+ Misses                   3027     3026       -1     
+ Partials                 1217     1216       -1     

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

@ericpre
Copy link
Member Author

ericpre commented Jun 6, 2024

The failure on CI is related to sympy 1.13.0rc1 - see sympy/sympy#26663 and potential fix in sympy/sympy#26678.

@bjodah
Copy link

bjodah commented Jun 6, 2024

@ericpre, I saw the mention in the PR over at SymPy: I'm curious, do you know what Derivative instances are present in the expressions you use lambdify on? If so, we should try to amend my PR against the sympy repo with some relevant test cases. In particular, I'd like to add test(s) to test_lambdify.py analogous to this one:
https://github.com/sympy/sympy/pull/26678/files#diff-a9bc959f22b154b22a706c75d4927a5a2806ce36f499b8e5b6f993470aebafa3

@ericpre ericpre force-pushed the support_nav_sig_axes_keyword branch from 5f7f406 to 28e5a8c Compare June 7, 2024 07:42
@ericpre
Copy link
Member Author

ericpre commented Jun 7, 2024

Thanks @bjodah, I will try to make a minimum example representation of the failure here in the coming days. From a quick look at the log, it most likely have to do with the computation of derivative of these expression:

expression="area * real(V); \
V = wofz(z) / (sqrt(2.0 * pi) * sigma); \
z = (x - centre + 1j * gamma_) / (sigma * sqrt(2.0))",

and
expression="A*heaviside(x-n,0.5)",

@ericpre ericpre force-pushed the support_nav_sig_axes_keyword branch 2 times, most recently from 2c0a081 to b584c87 Compare June 7, 2024 15:27
@ericpre
Copy link
Member Author

ericpre commented Jun 15, 2024

xref #814.

@ericpre
Copy link
Member Author

ericpre commented Jun 15, 2024

The test failure with sympy 1.13.0rc1 is fixed in #3388.

@CSSFrancis
Copy link
Member

CSSFrancis commented Jun 23, 2024

@ericpre I can look at this on Tuesday! I'm very excited for this one :)

+1 to following this up with something like what had been previously proposed.

s.axes_manager["nav"].scale=.2

s.axes_manager["nav"](scale=0.2, units="nm", offsets=0.2)

The second one looks kind of weird, although still correct and you could do s.axes_manager.signal_axes(...)

On another note there are so many more ways to index axes than I previously realized. It would be nice to have an example of how to use things like the imaginary number indexing indexing with the axis name etc in order to set things like the scale.

Maybe just a basic example showing how to sum data along different axes.

@ericpre
Copy link
Member Author

ericpre commented Jun 24, 2024

See #2756 for the setter.

Copy link
Member

@francisco-dlp francisco-dlp left a comment

Choose a reason for hiding this comment

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

LGTM!

hyperspy/docstrings/signal.py Outdated Show resolved Hide resolved
@ericpre ericpre force-pushed the support_nav_sig_axes_keyword branch from b584c87 to 2f1ca14 Compare June 25, 2024 08:45
@ericpre ericpre force-pushed the support_nav_sig_axes_keyword branch from 2f1ca14 to bf3e9cb Compare June 25, 2024 09:09
hyperspy/docstrings/signal.py Outdated Show resolved Hide resolved
hyperspy/docstrings/signal.py Outdated Show resolved Hide resolved
hyperspy/signal.py Show resolved Hide resolved
@jlaehne
Copy link
Contributor

jlaehne commented Jun 26, 2024

We should probably add a section on accessing the axes and their properties in the documentation on the Axes handling page right before "setting axis properties".

Co-authored-by: Jonas Lähnemann <jonas@pdi-berlin.de>
@ericpre
Copy link
Member Author

ericpre commented Jun 26, 2024

Yes, the documentation on AxesManager will need an overhaul and integrating with the gallery of example, etc. I was thinking that it would be a good time to do after #2756.

@ericpre ericpre force-pushed the support_nav_sig_axes_keyword branch from 8054d42 to daba5e9 Compare July 1, 2024 09:44
@ericpre ericpre merged commit abc0882 into hyperspy:RELEASE_next_minor Jul 1, 2024
26 of 27 checks passed
@ericpre ericpre deleted the support_nav_sig_axes_keyword branch July 3, 2024 18:55
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

5 participants