Add basic arithmetic methods

[wtbarnes]

This PR adds basic support for arithmetic methods on NDCube: add, subtract, multiply, divide. It explicitly does not support operations between NDCube objects (or more precisely anything that is an NDData instance).

There was also some discussion about to what extent we can support operations with Dask arrays. There is no issue if our NDCube is already backed by a Dask array. However, if the other object that we are performing the operation with is a Dask array, it will be eagerly evaluated because of the use of Quantity. This is probably not necessary and we should be checking possible other operands in a slightly different way.

TODOs

• Fix the tests
• Gallery example
• Changelog
• Create issues for supporting pow and rtruediv
• Create "downstream" issues on sunpy
• Test for NotImplementedError
• Scale uncertainties as part of multiplication

[pep8speaks]

Hello @wtbarnes! Thanks for opening this PR. We checked the lines you've touched for PEP 8 issues, and found:

• In the file ndcube/ndcube.py:

Line 817:36: W291 trailing whitespace

• In the file ndcube/tests/test_ndcube.py:

Line 869:80: E501 line too long (86 > 79 characters)
Line 894:80: E501 line too long (86 > 79 characters)
Line 910:80: E501 line too long (86 > 79 characters)
Line 928:80: E501 line too long (90 > 79 characters)

• In the file ndcube/utils/decorators.py:

Line 19:80: E501 line too long (93 > 79 characters)
Line 20:80: E501 line too long (96 > 79 characters)
Line 25:80: E501 line too long (97 > 79 characters)
Line 26:80: E501 line too long (96 > 79 characters)
Line 27:80: E501 line too long (96 > 79 characters)
Line 28:80: E501 line too long (93 > 79 characters)

[DanRyanIrish]

@wtbarnes: Summarising our discussion on the sunpy call:

• Current implementation of casting input to a Quantity forces input to be read into memory and not supportive of dask.
• Therefore, we should start by only supporting Quantities and non-unit objects, e.g. floats, arrays, etc. So the new implementation should be as follows:
  • Check if input has a unit using hasatrr(input, "unit")
  • If not, apply operation assuming input is a zero-uncertainty unitless scalar/array
  • If it does have a unit, check if input is a Quantity using isinstance.
  • If not, raise NotImplementedError
  • If so, confirm the unit is compatiable with the NDCube's unit. If not, raise Error.
  • Work out scaling factor between units.
  • Apply operation to data with input value * scaling factor. This should mean that the resulting data is still in the units of the original NDCube.
• In the future we can work out ways of being less restrictive the types we support. But this should account for many users' needs without painting ourselves into a corner and so is worth implementing.
• In particular, we should be able to support operations with NDData objects so long as they don't have a WCS, i.e. obj.wcs is None. See attempt to implement error propagation as part of Implement Superpixel on NDCube #450.

Feel free to push back if you feel this doesn't represent what we agreed.

[DanRyanIrish]

Any movement or planned movement on this @wtbarnes?

[wtbarnes]

Sorry I have not had any time to revisit this since the discussion you summarized above. I should have some time this week or next to make the refactor suggested in the above comment.

[wtbarnes]

The logic here is not very clean and could probably be refactored a bit to avoid some repetition, but I believe this accomplishes what is laid out above. Currently, I'm not explicitly excluding NDCube or NDData instances, but they're implicitly caught by the "has-unit-but-is-not-a-quantity" case.

[DanRyanIrish]

This is looking great @wtbarnes. I haven't reviewed the tests but have left a few comments and one discussion point (about NDCube vs NDCubeBase). This PR isn't the end of the story but is a big leap forward and I believe isn't far off now.

The way you've filtered out the NDCube and NDData instances is a good way to go.

[Cadair]

I still need to review this properly, but I want to keep the behaviour of NDCube and Map the same (in all things) at the moment, so if we are going to deviate from map for NotImplemeted vs NotImplementedError then I would want a PR open to sunpy core with the same change before we merge this one.

[ayshih]

To be clear, the deviation from Map is not NotImplemented versus NotImplementedError, because Map does not return either for the situation under discussion. If there are uncertainties on the data, Map doesn't care at all, and in fact discards any uncertainties in its arithmetic operations.

[DanRyanIrish]

After discussion with @ayshih we identified the following remaining issues.

• In fact, unsupported type combinations for the operations should return NotImplemented, not NotImplementedError. NotImplemented allows other types to override the operators to handle NDCubes which we should allow while NotImplementedError does not. Moreover, this would not require a fix in sunpy to maintain alignment with Map.
• A point to highlight that does not require a change to this PR: There is divergence between this PR and Map in that M̀ap always drops uncertainties when any operation is applied. This PR does not do that. This is a shortcoming of Map that would be addressed by rebasing onto NDCube. Map suddenly not dropping uncertainties should cause no issues as any code written for Maps without uncertainties should not break if there are uncertainties.

I will leave a few code suggestions in inline comments that should address the above issues.

[wtbarnes]

Just one minor correction in a comment, but otherwise I think this is ready to go

Issues raised in review have been resolved

[DanRyanIrish]

Thanks so much @wtbarnes for this!!!!