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

_assign_subclass: signal_shape (1,) now becomes BaseSignal #2773

Merged
merged 11 commits into from Sep 5, 2021

Conversation

magnunor
Copy link
Contributor

@magnunor magnunor commented Jun 18, 2021

As discussed in #2766, currently a signal with signal dimensions (1,) is assigned to be a Signal1D. This is not optimal, as most methods which apply to Signal1D probably won't work very well if such 1-value signals.

So this pull request changes how _assign_subclass works, by now making signals with signal dimension of (1,) BaseSignal instead of Signal1D.

Progress of the PR

  • Change implemented
  • 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

import hyperspy.api as hs
import numpy as np
s = hs.signals.Signal1D(1) # <Signal1D, title: , dimensions: (|1)>
s._assign_subclass() # <BaseSignal, title: , dimensions: (|1)>

@codecov
Copy link

codecov bot commented Jun 18, 2021

Codecov Report

Merging #2773 (12f1c58) into RELEASE_next_patch (6700537) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@                 Coverage Diff                 @@
##           RELEASE_next_patch    #2773   +/-   ##
===================================================
  Coverage               77.37%   77.38%           
===================================================
  Files                     202      202           
  Lines                   30108    30111    +3     
  Branches                 6579     6580    +1     
===================================================
+ Hits                    23295    23300    +5     
+ Misses                   5066     5065    -1     
+ Partials                 1747     1746    -1     
Impacted Files Coverage Δ
hyperspy/axes.py 86.34% <100.00%> (ø)
hyperspy/misc/signal_tools.py 92.30% <100.00%> (ø)
hyperspy/signal.py 76.24% <100.00%> (+0.03%) ⬆️
hyperspy/io.py 84.18% <0.00%> (+0.79%) ⬆️

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 6700537...12f1c58. Read the comment docs.

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.

The signal_dimension is not set correctly, for example:

import hyperspy.api as hs

s = hs.signals.Signal1D(1)
s._assign_subclass()
print(s.__class__.__name__, 'with signal dimension:', s.axes_manager.signal_dimension)

s has a signal dimension of 1, while it should be zero.

@magnunor
Copy link
Contributor Author

When first making this pull request, I wasn't really sure which avenue to take. Either via changing how the AxesManager works, or by changing how _assign_subclass works.

The original implementation changed how _assign_subclass worked, which I think is the less "risky" way of doing it. Since it much less changes the functionality. I.e. less odds of something breaking in subtle ways. However, the downside is that (some) base signals would still report having 1 signal dimension.

The new implementation, changing how the AxesManager object sets the signal_dimensions, has way larger effects. Possible unintended.

In fact, the "new" implementation should really go way deeper, since now it only changes how signal_dimension is handled. While ignoring axes_manager.signal_axes.

I'm not really sure which way to go with this, since I feel that disallowing BaseSignal with signal axes is potentially a lot of work.

@ericpre
Copy link
Member

ericpre commented Jun 20, 2021

Previous discussion on this topic: #643.

The original implementation changed how _assign_subclass worked, which I think is the less "risky" way of doing it. Since it much less changes the functionality. I.e. less odds of something breaking in subtle ways. However, the downside is that (some) base signals would still report having 1 signal dimension.

In which case, things requiring signal_dimension == 1 may be broken because the signal_dimension value will be incorrect.

In fact, the "new" implementation should really go way deeper, since now it only changes how signal_dimension is handled. While ignoring axes_manager.signal_axes.

What do you mean with ignoring axes_manager.signal_axes?

I'm not really sure which way to go with this, since I feel that disallowing BaseSignal with signal axes is potentially a lot of work.

I don't understand why BaseSignal with signal axes would be disallowed?

hyperspy/axes.py Outdated Show resolved Hide resolved
@magnunor
Copy link
Contributor Author

magnunor commented Jul 9, 2021

These signals will now return signal_dimension=0. However, I'm not sure how best to handle some of the functionality which breaks. For example:

import numpy as np
import hyperspy.api as hs
s = hs.signals.Signal1D([1])
s1 = hs.signals.Signal1D([2])
hs.stack([s, s1])

Gives the error:

hyperspy/misc/utils.py in stack(signal_list, axis, new_axis_name, lazy, stack_metadata, show_progressbar, **kwargs)
   1022     if len(signal_list) > 1:
   1023         # Matching axis calibration is checked here
-> 1024         newlist = broadcast_signals(*signal_list, ignore_axis=axis_input)
   1025 
   1026         if axis is not None:

hyperspy/misc/signal_tools.py in broadcast_signals(ignore_axis, *args)
    188                     data = np.broadcast_to(data, thisshape)
    189                 else:
--> 190                     data = da.broadcast_to(data, thisshape)
    191 
    192             ns = s._deepcopy_with_new_data(data)

dask/array/core.py in broadcast_to(x, shape, chunks)
   4349         new != old for new, old in zip(shape[ndim_new:], x.shape) if old != 1
   4350     ):
-> 4351         raise ValueError("cannot broadcast shape %s to shape %s" % (x.shape, shape))
   4352 
   4353     if chunks is None:

ValueError: cannot broadcast shape (1, 1) to shape (1,)

It also breaks the following test:

import numpy as np
import hyperspy.api as hs
s32 = hs.signals.Signal1D(np.ones((2, 3)))
s12 = hs.signals.Signal1D(np.ones((2, 1)))
print(s32.axes_manager.signal_dimension) # 1 signal dim
print(s12.axes_manager.signal_dimension) # 0 signal dim
s32 + s12

Gives the error

ValueError                                Traceback (most recent call last)
002_test_script.py in <module>
      5 print(s32.axes_manager.signal_dimension)
      6 print(s12.axes_manager.signal_dimension)
----> 7 s32 + s12

hyperspy/signal.py in __add__(self, other)

hyperspy/signal.py in _binary_operator_ruler(self, other, op_name)
   2253                 else:
   2254                     # They are broadcastable but have different number of axes
-> 2255                     ns, no = broadcast_signals(self, other)
   2256                     sdata = ns.data
   2257                     odata = no.data

hyperspy/misc/signal_tools.py in broadcast_signals(ignore_axis, *args)
    186             if data.shape != thisshape:
    187                 if isinstance(data, np.ndarray):
--> 188                     data = np.broadcast_to(data, thisshape)
    189                 else:
    190                     data = da.broadcast_to(data, thisshape)

<__array_function__ internals> in broadcast_to(*args, **kwargs)

numpy/lib/stride_tricks.py in broadcast_to(array, shape, subok)
    178            [1, 2, 3]])
    179     """
--> 180     return _broadcast_to(array, shape, subok=subok, readonly=True)
    181 
    182 

numpy/lib/stride_tricks.py in _broadcast_to(array, shape, subok, readonly)
    121                          'negative')
    122     extras = []
--> 123     it = np.nditer(
    124         (array,), flags=['multi_index', 'refs_ok', 'zerosize_ok'] + extras,
    125         op_flags=['readonly'], itershape=shape, order='C')

ValueError: input operand has more dimensions than allowed by the axis remapping

Which happens in https://github.com/hyperspy/hyperspy/blob/RELEASE_next_minor/hyperspy/misc/signal_tools.py#L190

One solution could be to add if else in hyperspy/misc/signal_tools/broadcast_signals for this specific situation, ergo to check if the signal dimension is 0, then if the data size is 1, then put signal dimension to 1 inside the functions. But not sure if doing that is the best solution: that function is a bit difficult to follow, so I'm a bit hesitant in touching it.

@ericpre
Copy link
Member

ericpre commented Jul 12, 2021

@magnunor, I made a PR to fix the issues of this PR, please have a look.

@magnunor
Copy link
Contributor Author

magnunor commented Sep 5, 2021

@ericpre, that seems to have fixed the issue!

Build doc is failing, but I think that is unrelated.

Ready for review!

@ericpre ericpre merged commit 11f26a7 into hyperspy:RELEASE_next_patch Sep 5, 2021
@ericpre ericpre mentioned this pull request Oct 24, 2021
8 tasks
ericpre added a commit to ericpre/hyperspy that referenced this pull request Oct 24, 2021
…_value"

This reverts commit 11f26a7, reversing
changes made to 6700537.
ericpre added a commit to ericpre/hyperspy that referenced this pull request Oct 25, 2021
…_value"

This reverts commit 11f26a7, reversing
changes made to 6700537.
@ericpre ericpre mentioned this pull request Oct 26, 2021
6 tasks
@ericpre ericpre added this to the v1.6.5 milestone Apr 26, 2022
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