Implent NotImplemented case in Spectrum1D subtraction

[ojustino]

Hello, this is an update to Spectrum1D's subtraction operator. I'm coming from astropy/specreduce#144, where we are improving specreduce's compatibility with Spectrum1D objects.

A feature we'd like to support is the ability to subtract a Spectrum1D object by a specreduce Background object. That currently fails since Spectrum1D.__sub__ tries to call u.Quantity() on operands that aren't of type NDCube and that causes a TypeError with Background.

My proposal catches the error and returns NotImplemented in order to pass the operation to the other operand's __rsub__ method. If the other object lacks this operator, a general TypeError is raised saying that subtraction isn't supported between the two types. If there's an error in the other object's __rsub__ method, only that is reported (no mention of NotImplemented in Spectrum1D.__sub__).

Is this relevant enough to mention in the documentation? We only need subtraction for specreduce, but should this behavior extend to other operators?

[rosteen]

Thanks! I think the arithmetic handling is due for a bit of a larger overhaul to ensure good handling, but this is a good start. I do think that this would be good to apply to the other operators that try to turn the other operand into a Quantity - do you want to add that here, or have me do it in a followup?

[ojustino]

It looks like the change to Spectrum1D.__mul__ is what's causing the test failures. The error (comparing one flux in MJy and the other in MJy**2) reminds me something @pllim encountered a couple of months ago: astropy/specreduce#130 (comment). I was never able to reproduce it, but the DN in the linked case is because specreduce mostly assumes unitless fluxes to be that unit.

Would you rather troubleshoot the failure here or just apply this change to all operations except multiplication?

[pllim]

I think that when you return NotImplemented, it will look for the operator in other, I don't grok the codebase here so just want to make sure that is really what you want, because it might return something surprising.

[pllim]

Shouldn't multiplication and division only be allowed if other is unitless (like a bandpass). Otherwise, the resultant flux unit will be nonsense.

[rosteen]

I think that when you return NotImplemented, it will look for the operator in other, I don't grok the codebase here so just want to make sure that is really what you want, because it might return something surprising.

Yup, that's exactly what was wanted here, so that it will fall back to the specreduce (or anyone else who implements classes that interact with Spectrum1D in defined way) subtraction handling when this fails.

[rosteen]

It looks like the change to Spectrum1D.__mul__ is what's causing the test failures. The error (comparing one flux in MJy and the other in MJy**2) reminds me something @pllim encountered a couple of months ago: astropy/specreduce#130 (comment). I was never able to reproduce it, but the DN in the linked case is because specreduce mostly assumes unitless fluxes to be that unit.

Would you rather troubleshoot the failure here or just apply this change to all operations except multiplication?

If multiplication is causing errors (and given the thoughts @pllim had about multiplication and division), let's just apply this to addition and subtraction for now where we know it makes sense and isn't causing errors. The arithmetic handling in Spectrum1D is due for a refresh/improvements soon, I can think more deeply about these questions when I get a chance to work on that.

[rosteen]

Test failure looks like an unrelated remote data download timeout. Thanks!

[pllim]

You don't really need the __add__ for specreduce, right? I only found __rsub__ in specreduce.