Signal1D/2D tools consolidation

[dnjohnstone]

An alternative to #961 to address #943

That PR really breaks everything - the reason being that it creates a situation where signal.py needs to import from /_signals/signal1D.py and vice-versa resulting in nasty cyclic import issues I think.

The easiest way to get round this is to rather than putting all the signal1D_tools into signal1D instead create /misc/signal1D_tools.py that includes the old spectrum_tools.py. Whilst this doesn't reduce the number of files as much as the previous attempt it is intuitive and results in 5 (signal.py, signal1D.py, signal2D.py, signal1D_tools.py, and signal2D_tools.py) much more tolerably sized files - which was the primary aim of this endeavour.

Since this works and makes a lot of sense to me - I think this is the way to go... opinions elsewhere?

[dnjohnstone]

By the way - performing the renaming Spectrum --> Signal1D and Image --> Signal2D seems to be much more painful... I can't make that work without breaking a vast number of tests that I think basically don't like the change in API...

As it stands - I'm starting to wonder if deprecating Spectrum/Image is a good idea at all? This PR achieves the primary goal of reducing the bloating of signal.py and it also achieves the second most important goal of consolidating tools for 1D & 2D signals into one place so that there is only one place to look for that.

The renaming is now just that, a renaming and I think that many people will complain that Signal1D/Signal2D are less intuitive names than Spectrum/Image -- especially if you're a microscopist!

On the other hand, if we're ever going to make that change it wants to be ASAP as it will only get more painful so should be done before 1.0.0 if we're going to go for it.

@francisco-dlp @to266 opinions would be most welcomed

[to266]

I'm fine either way. However, it's not really clear to me if we want to rename everything: e.g. methods as_spectrum and similar? Also, since this seems to be aimed to be released before 1.0.0, then do we want to change model.spectrum -> model.signal1D?

Essentially lots of small things that are not immediately clear to me.

Or do we just want to remove the ability to create new signals using spectrum and image keywords, and use signal1D and signal2D instead?

[magnunor]

I'm in favor of splitting it up like that, which should (?) make things a bit more orderly. And in my head, it should make it easier to create more specialized classes which inherit either signal1D or signal2D. For example, making STEMImage or DiffractionImage classes which include useful functions for analysing those kinds of signals.

[dnjohnstone]

Ok - I discussed this with @francisco-dlp in person yesterday and I thinkwe broadly agree with @magnunor that this change will put everything on a 'more general' footing moving forward. In the current version of this PR I've renamed Spectrum --> Signal1D and Image -->Signal2D as you can see it breaks an absolute tonne of tests e.g. anything that checks something is in the Signal class because that no longer exists.

I will plod through all of this eventually, but if anyone feels like helping it will be much appreciated.

@to266 I agree all of that is unclear, but I think the idea is that it should all be renamed too.

[dnjohnstone]

ok @francisco-dlp this is getting pretty close now... would be a good moment if you can have a look through to let me know if I've missed anything major!

[dnjohnstone]

Also note I moved some things from Signal2DTools to drawing.utils because not doing so resulted in a cyclic import mess - since it's only used there I think this is ok.

[dnjohnstone]

I'm also not totally sure why coverage is decreasing...

[francisco-dlp]

Looks very good!

Some comments:

• Signal should be renamed to BaseSignal
• The old Signal should be recreated as class Signal(BaseSignal, Signal1DTools, Signal2DTools) with a deprecation warning.
• BaseSignal, Signal1D, and Signal2D should be available in hyperspy.api.signals
• to_spectrum, to_image, as_spectrum and as_image must be renamed, and new methods with the same name created with deprecation warnings.
• The image and spectrum tests should be renamed and test Signal1D and Signal2D instead.
• The documentation (user guide and docstrings) should:
  • Use Signal1D and Signal2D instead of Spectrum and Image
  • to_signal1D, to_signal2D, as_Signal2D and as_signal1D instead of their old selves.
  • Inform that Spectrum, Image, and Signal are deprecated and encourage the reader to update his scripts.
  • Sentences like "These methods are only available for Signal object with signal_dimension equal to one." require updating.

[dnjohnstone]

@francisco-dlp the issue with having signal.Signal as you describe is that it creates a cyclic import with signal1D and signal2D.... The only way I can see to resolve this is to make separate .py files for signal and base signal.

in the end I think we want the API to have signal.BaseSignal but I'm not sure if I'm allowed to do that and then create signal_old.Signal to give the deprecation warning? Otherwise there will be another rename needed when merging with master.... what do you prefer?

[francisco-dlp]

Could you report the marker problem in a separate issue?

[dnjohnstone]

#980

[to266]

I haven't finished looking into this, but a couple of comments:

• I find it unintuitive / inelegant that when picking as_signal1D and as_signal2D instead of the as_spectrum and as_image, they both return a Spectrum and Image respectively, and then complain that you should use signal1D and signal2D. I understand that it's only for the 0.8.x version, to give people some time to adjust, but I'm not a fan :)
• Do we keep the EELSSpectrum and all the EDS*EMSpectrum classes? Will those get renamed to EELSSignal1D, etc.?

[dnjohnstone]

EELSSpectrum & EDS*Spectrum definitely stay - because that's what they are... The point is that now if someone has say an PowderXRD time series (which would be a good thing to analyse with HyperSpy) it can inherit from Signal1D which makes sense as it is a one-dimensional signal rather than from Spectrum, which doesn't make so much sense as it's not really a Spectrum in any sense.

As regards as_stuff methods -- in principle I agree. However, I think we should address #974 at this point (or in a separate PR) rather than spending time making record_by consistent for everything in 0.8.5 only to remove it...

[francisco-dlp]

@dnjohnstone, although we agreed above on the current solution for the as_* and to_* methods, the fact that @to266 is not happy with it has made me change my mind. Actually, it'll be easy to make this work without addressing the record_by issue. to_sig* and as_sig* could call a new assign_signal_subclass method which could be exactly as the current one but deleting Signal, Spectrum and Image from the dictionary instead of BaseSignal, Signal1D and Signal2D. Obviously this new method is just a temporary hack and we'll delete when merging into master.

Beside, the docstrings of the as_signal* and to_signal* methods were not updated to the new nomenclature. Some keyword arguments (e.g. image_axes) many need updating too.

[to266]

Also it seems mva/test_decomposition.py, signal/test_apply_function.py, signal/test_krames_kronig_transform.py and signal/test_binned.py still have calls to hs.signals.Signal that should (I assume) be replaced with hs.signals.BaseSignal

[francisco-dlp]

I've just noticed that model.spectrum has been renamed to model.signal1D. Two comments:

• Wouldn't simply model.signal be a better name (to be consistent with master)?
• Nevertheless, we can't break the API, so model.spectrum should remain in place and functional, but raise a deprecation warning.

[dnjohnstone]

Ok - I've made the requested updates to tests because they're easy.

model.spectrum --> model.signal1D: Also easy and I've done locally, but just for clarity. The point is that in master it should become model.signal1D for model1D and model.signal2D for model2D. (in master it is model.spectrum and model.image right now) I thought that if I change it like this now then for 1.0.0 all that would be new is model.signal2D. (Note that the merge for this will be a pain, but not so bad) Fine if we need a model.spectrum for 0.8.5 - but is my overall thinking correct here?

record_by needs a little more careful thought --> what you suggest @francisco-dlp doesn't work easily because Signal1D would need to have record_by "signal1D" but Spectrum, which inherits from Signal1D, would need to have record_by "spectrum". There are also many tests that check this attribute is "spectrum" or "image".

It's a little unclear to me whether I should just change all of these to "signal1D" and "signal2D", which will appear to fix the problem short term but I don't think is a good idea if we're then going to change that. (Also whilst record_by spectrum and record_by image have some historical sense in EM record_by signal1D etc is a very odd attribute to have in my mind). Really I think we need to have a clear plan for record_by - if we're going to remove it, or use it differently for version 1.0.0 then we should work out what we're going to do as soon as possible and make the change in this PR consistent with that plan.... With all this in mind I really prefer the current (if slightly) ugly, solution because it's soooo much easier and changes the usage minimally, which is probably good for 0.8.5. To me gentle hints that we're moving away from it but nothing breaking if you for some reason set record_by = spectrum for example is a good thing. What do you think - to me it's all about what we want to do with record_by beyond this PR? @to266 & @francisco-dlp

[dnjohnstone]

This now has previous points addressed (I think) aside from the record_by question - happy with whatever the solution settled on there is, but just need to know...

[francisco-dlp]

Once you merge 0.8.x into this I merge it. I've just merged 0.8.x into master to easy the merge of this into master.

[dnjohnstone]

Ok - I think this is ready then....