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 __setattr__() method to AxesManager's signal_axes and navigation_axes #2756

Draft
wants to merge 5 commits into
base: RELEASE_next_minor
Choose a base branch
from

Conversation

francisco-dlp
Copy link
Member

The goal is to ease the task of setting up the axes attributes, see below for an example.

The syntax is inspired by (but different from) https://matplotlib.org/stable/users/whats_new.html#axes-spines-access-shortcuts

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),
  • 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.random.random((10, 20, 30)))
>>> s.axes_manager.navigation_axes.name = ("x", "y")
>>> s.axes_manager.navigation_axes.scale = 1e-3
>>> s.axes_manager.navigation_axes.units = "nm"
>>> s.axes_manager.signal_axes.name = "Energy loss"
>>> s.axes_manager.signal_axes.units = "eV"
>>> s.axes_manager.signal_axes.offset = 300
s.axes_manger
<Axes manager, axes: (20, 10|30)>
            Name |   size |  index |  offset |   scale |  units 
================ | ====== | ====== | ======= | ======= | ====== 
               x |     20 |      0 |       0 |   0.001 |     nm 
               y |     10 |      0 |       0 |   0.001 |     nm 
---------------- | ------ | ------ | ------- | ------- | ------ 
     Energy loss |     30 |        |   3e+02 |       1 |     eV

@francisco-dlp francisco-dlp marked this pull request as draft May 28, 2021 08:52
@francisco-dlp francisco-dlp changed the title Add __setitem__() method to AxesManager's signal_axes and navigation_axes Add __setattr__() method to AxesManager's signal_axes and navigation_axes May 28, 2021
@francisco-dlp
Copy link
Member Author

francisco-dlp commented May 28, 2021

This is WIP. I will complete it once I get feedback on the syntax and functionality. Some notes:

  • Instead of __setattr__ we could use a standard method e.g. set_attributes(). That comes with the advantage of being more standard (the unusual usage of the __setattr__ syntax may lead some to confusion) and enabling documentation through docstring. However, it is less convenient.
  • This could also be useful for Model.components e.g.:
    m.components.select("GaussianHF").name = ("peak1", "peak2", "peak3")
    m.components.select(("peak1", "peak3")).fwhm = 2.6
    However, that would require an even less standard syntax: notice how the last line sets the fwhm.value attribute without specifying value...

So, should convenience win over orthodoxy? If no, can you come up with an orthodox and equally convenient syntax?

@codecov
Copy link

codecov bot commented May 28, 2021

Codecov Report

Merging #2756 (368fba6) into RELEASE_next_minor (ea89fe2) will increase coverage by 0.04%.
The diff coverage is 60.86%.

Impacted file tree graph

@@                  Coverage Diff                   @@
##           RELEASE_next_minor    #2756      +/-   ##
======================================================
+ Coverage               77.69%   77.73%   +0.04%     
======================================================
  Files                     203      203              
  Lines                   30627    30652      +25     
  Branches                 6670     6676       +6     
======================================================
+ Hits                    23796    23828      +32     
+ Misses                   5051     5043       -8     
- Partials                 1780     1781       +1     
Impacted Files Coverage Δ
hyperspy/misc/utils.py 85.21% <55.00%> (-1.06%) ⬇️
hyperspy/axes.py 86.74% <100.00%> (ø)
hyperspy/io.py 82.94% <0.00%> (-0.78%) ⬇️
hyperspy/_signals/signal1d.py 77.09% <0.00%> (-0.28%) ⬇️
hyperspy/misc/example_signals_loading.py 100.00% <0.00%> (ø)
hyperspy/drawing/signal1d.py 78.01% <0.00%> (+0.90%) ⬆️
hyperspy/signal_tools.py 69.25% <0.00%> (+1.22%) ⬆️

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 ea89fe2...368fba6. Read the comment docs.

@ericpre
Copy link
Member

ericpre commented May 31, 2021

This will be indeed very convenient and this would nicely replace the following usage (which I do quite regularly):

for axis, name in zip(s.axes_manager.signal_axes, ("x", "y")):
   axis.name = name

If we want to keep it standard (not changing __setattr__), we can simply use the following syntax:

s.axes_manager.navigation_axes[:].name = ("x", "y")

Regarding m.components, I agree that it would be very nice too. Maybe consider renaming select by filter. Not specifying value seems to be slightly off outside of the scope of this PR.

@ericpre
Copy link
Member

ericpre commented May 31, 2021

Since this PR will make more easy to set units and scale of the axis, I think that it would address #2237.

There is a related issue on the topic of indexing the axes_manager: #814

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