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

Added new features to the ndcube.__add__ method #794

Open
wants to merge 50 commits into
base: main
Choose a base branch
from

Conversation

PCJY
Copy link
Contributor

@PCJY PCJY commented Dec 11, 2024

PR Description

write down the scenarios.

(list, to see what has been covered)

Testing scenarios without mask

redrafting the tests.

  • One and only one of them has a unit. [Error]

  • Both NDCube and NDData have unit
    Both have uncertainty.
    NDCube has uncertainty.
    NDData has uncertainty.
    Neither has uncertainty.

  • Neither has a unit.
    Both have uncertainty.
    NDCube has uncertainty.
    NDData has uncertainty.
    Neither has uncertainty.

name them again:
test_cube_add_cube_unit_mask_nddata_unc_unit_mask

ndcube/ndcube.py Outdated
Comment on lines 930 to 931
# addition
new_data = self.data + value_data
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The addition should be done as part of the masked array addition. You've already done this below, you just need to extract the added data from the results as well as the mask.

ndcube/ndcube.py Outdated
Comment on lines 950 to 952
return self._new_instance(
data=new_data, uncertainty=new_uncertainty, mask=new_mask
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of having a separate return here for the NDData case, I think we should build a dictionary of kwargs that we can give self._new_instance, here. So, you can create an empty kwargs dictionary at the start of the method, and add the new data, uncertainty, etc. in the relevant place, e.g.

kwargs["uncertainty"] = new_uncertainty

Then the final line of the method would become

return self._new_instance(**kwargs)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know if this doesn't make sense

@nabobalis nabobalis added this to the 2.4.0 milestone Dec 18, 2024
ndcube/ndcube.py Outdated
if self.uncertainty is not None and value.uncertainty is not None:
new_uncertainty = self.uncertainty.propagate(
np.add, value.uncertainty, correlation=0
np.add, value.uncertainty, result_data = value.data, correlation=0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The result_data needs to be the result of the operation. So, assuming you moved the addition of the datas using the masked array to before the uncertainty propagation, you could do:

Suggested change
np.add, value.uncertainty, result_data = value.data, correlation=0
np.add, value.uncertainty, result_data = kwargs["data"], correlation=0

ndcube/ndcube.py Outdated
Comment on lines 1061 to 1070
# combine mask
self_ma = np.ma.MaskedArray(self.data, mask=self.mask)
value_ma = np.ma.MaskedArray(value_data, mask=value.mask)

# addition
result_ma = self_ma + value_ma
new_mask = result_ma.mask

# extract new mask and new data
kwargs["mask"] = result_ma.mask
kwargs["data"] = result_ma.data
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned in above comment, I think it makes sense to do this before the uncertainty propagation so you can use the kwargs["data"] value in that propagation.

ndcube/ndcube.py Outdated
kwargs["data"] = result_ma.data

# return the new NDCube instance
return self._new_instance(**kwargs)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this line to the end of the method and use the kwargs approach when handling the other cases, e.g. Quantity. So, for example, L1082 would become:

kwargs["data"] = self.data + value.to_value(cube_unit)

@DanRyanIrish
Copy link
Member

A changelog file needs to be added.

And your branch needs to be updated with the latest version of main.

@PCJY
Copy link
Contributor Author

PCJY commented Jan 18, 2025

Hi @DanRyanIrish, as we have discussed in our project meetings, below are the issues we encountered and may need further discussions with others in the community:

The issue is mainly around how NumPy handles masks when performing an addition for two NumPy.MaskedArray.
We think the expected outcome for an addition should be: the sum of any value that is not masked by its individual mask.
E.g. [1] ([T]) + [2] ([F]) = [2].

However, from experimentation, it can be seen that
NumPy returns in this way:
[1] ([T]) + [2] ([F]) = [1].
Screenshot 2025-01-18 220004

I find this confusing because even if it does combine the mask and then apply it on the result, it should be:
[1] ([T]) + [2] ([F]) = [-].

Please correct me if there is anything wrong in my understanding.

@PCJY
Copy link
Contributor Author

PCJY commented Jan 18, 2025

@DanRyanIrish, Secondly, we also encountered some issues around the propagate method:
it ignores the mask of the objects that are passed in, and still takes into account the uncertainties of the masked elements when it should not have done so.
Following your guidance and suggestions, this issue is currently being worked on by setting the corresponding entries that should be masked of the uncertainty array to be 0, before passed in to the propagate method.
A clearer example of the issue was implemented as shown in the code below with the screenshot of its output attached.

from ndcube import NDCube
import numpy as np
from astropy.nddata import StdDevUncertainty
from astropy.wcs import WCS

data = np.array([[1, 2], [3, 4]])  
uncertainty = StdDevUncertainty(np.array([[0.1, 0.2], [0.3, 0.4]])) 
mask = np.array([[False, True], [False, False]])  
wcs1 = WCS(naxis=2) 
wcs1.wcs.ctype = ["HPLT-TAN", "HPLN-TAN"]

cube = NDCube(data, wcs=wcs1, uncertainty=uncertainty, mask=mask)
print(cube)

def add_operation(cube1, cube2):
    """
    Example function to add two data arrays with uncertainty propagation.
    """
    result_data = cube1.data + cube2.data 
    # Propagate the uncertainties using the NDCube objects
    propagated_uncertainty = cube1.uncertainty.propagate(
        np.add, cube2, result_data=result_data, correlation = 0
    )
    return result_data, propagated_uncertainty

# adding the cube to itself
result_data, propagated_uncertainty = add_operation(cube, cube)

print("Original Data:\n", cube.data)
print("Original Uncertainty:\n", cube.uncertainty.array)
print("Result Data (after addition):\n", result_data)
print("Propagated Uncertainty:\n", propagated_uncertainty.array)

Screenshot 2025-01-18 222146

@DanRyanIrish
Copy link
Member

DanRyanIrish commented Jan 20, 2025

Hi @PCJY. I think the first thing we need to do is decide what behaviours we want to implement in the case where at least one of the NDCube and NDData have a mask. I think we need to get some feedback from other users on this decision. I propose the following scheme (@Cadair, thoughts on this?):

Firstly, if an object has no mask, that is equivalent to all pixels being unmasked.
Secondly, for a given pixel in both objects:

  1. If both are unmasked, the resultant
    i. data value is the sum of both pixels
    ii. mask value is False
    iii. uncertainty value is the propagation of the two uncertainties. If one or other object doesn't have uncertainty, the uncertainty of that component is assumed to be 0.
  2. If it is masked in one object, but not the other, the resultant:
    i. data value is equal to the unmasked value
    ii. mask value is False
    iii. uncertainty value is the same as the unmasked pixel
  3. If both pixels are masked, this is where is gets ambiguous. I propose, in order to remain consistent with the above:
    i. The operation is not performed and the data, mask and uncertainty values remain the same as the left-hand operand, i.e. the NDCube.

Alternatives for parts of the scheme could include:
2. If it is masked in one object, but not the other, the resultant:
ii. mask value is True.

  1. If both pixels are masked:
    i. The operation IS performed as normal but the mask value is True.

Once we agree on a scheme, the way forward on your uncertainty questions will become clear.

@Cadair what are your thoughts on this scheme. I also think we should bring this up at the sunpy weekly meeting to get other thoughts.

@DanRyanIrish
Copy link
Member

I find this confusing because even if it does combine the mask and then apply it on the result, it should be: [1] ([T]) + [2] ([F]) = [-].

This is where I also find numpy masked arrays counter-intuitive. However, the logic is as follows:

  • If one pixel is masked, retain the data of the left-hand operand and set the mask value to True.
    Notice that order of the operands matters. Because you did [1] ([T]) + [2] ([F]), the results is [1] ([T]), which is displayed as [--]. I would expect if you did the operation the other way around ([2] ([F]) + [1] ([T])), the result would be [2] ([T]).

Notice that this is not the same as the scheme I've proposed in my previous comment, in part because it's confusing, as you've found.

@DanRyanIrish
Copy link
Member

DanRyanIrish commented Jan 20, 2025

@PCJY, until we agree a way forward with the mask, you should proceed by implementing in the case for which neither object has a mask. So no need for masked arrays.

@Cadair
Copy link
Member

Cadair commented Jan 21, 2025

I propose the following scheme

I haven't thought too much each of these individual cases, but the fact there is a list is enough to make me think we probably need a way for the user to choose. This is obviously not possible with the __add__ operator, so we would need to have a NDCube.add method (and presumably subtract) which accepted kwargs.

Is this also not a problem for other operators as well?

@DanRyanIrish
Copy link
Member

I think this is a good idea. As well as add and subtract, I think we would also need multiply and divide methods.

As far as I can see, this ambiguity only arises when there are masks involved. So we could still implement the dunder methods as wrappers around the above methods, but have the raise and error/return NotImplemented if the non-NDCube operand has a mask, and require users use the NDCube.add method instead.

@DanRyanIrish
Copy link
Member

This is looking good. The test_cube_add_uncertainty should also check the other attributes are as expected, i.e., the data and the unit. For this, it might be better to not initiate your fixture with np.random.rand, but a known set of values, e.g. np.ones.

Co-authored-by: DanRyanIrish <[email protected]>
ndcube/ndcube.py Outdated
@@ -987,7 +987,7 @@ def add(self, value, operation_ignores_mask=True, handle_mask=np.logical_and):

if (self_unmasked and value_unmasked) or operation_ignores_mask is True:
# addition
kwargs["data"] = self.data + value_data
kwargs["data"] = (self.data + value_data)*self.unit
Copy link
Member

@DanRyanIrish DanRyanIrish Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
kwargs["data"] = (self.data + value_data)*self.unit
kwargs["data"] = self.data + value_data

@PCJY
Copy link
Contributor Author

PCJY commented Feb 20, 2025

Notes for the current stage of development:
1, The 'TypeError: None is not a valid Unit' occurs whenever an NDCube/NDData object with a unit enters the propagate method.
Their values become None after they enter the propagate method. Their values remain correct before entering the propagate method.
It might happen due to how the unit is handled or converted inside the propagate method.

The stack trace output of pdb w tells me:

the test function performs the addition, the dunder add method is called,
inside the dunder add method, the propagate method is called,
it then calls the _propagate_add method within the astropy nduncertainty file,
which then calls the _propagate_add_sub method,
which calls the method .to() for the result_unit_sq,
then, inside the .to() method, it tries to convert the unit to the unit of the argument, but finds that unit of result_unit_sq is None.
the __call__ method inside the core file is called and raises a TypeError.

result_unit_sq is not set correctly, in _propagate_add_sub() .

result_unit_sq is None, but other objects do have values:

> c:\users\junya\anaconda3\envs\ndcube-dev\lib\site-packages\astropy\nddata\nduncertainty.py(717)_propagate_add_sub()
-> .to(result_unit_sq)
(Pdb) locals()
{'self': StdDevUncertainty([[0.2, 0.2, 0.2, 0.2, 0.2, 0.2, 0.2, 0.2, 0.2, 0.2, 0.2,
                    0.2],
                   [0.2, 0.2, 0.2, 0.2, 0.2, 0.2, 0.2, 0.2, 0.2, 0.2, 0.2,
                    0.2],
                   [0.2, 0.2, 0.2, 0.2, 0.2, 0.2, 0.2, 0.2, 0.2, 0.2, 0.2,
                    0.2],
                   [0.2, 0.2, 0.2, 0.2, 0.2, 0.2, 0.2, 0.2, 0.2, 0.2, 0.2,
                    0.2],
                   [0.2, 0.2, 0.2, 0.2, 0.2, 0.2, 0.2, 0.2, 0.2, 0.2, 0.2,
                    0.2],
                   [0.2, 0.2, 0.2, 0.2, 0.2, 0.2, 0.2, 0.2, 0.2, 0.2, 0.2,
                    0.2],
                   [0.2, 0.2, 0.2, 0.2, 0.2, 0.2, 0.2, 0.2, 0.2, 0.2, 0.2,
                    0.2],
                   [0.2, 0.2, 0.2, 0.2, 0.2, 0.2, 0.2, 0.2, 0.2, 0.2, 0.2,
                    0.2],
                   [0.2, 0.2, 0.2, 0.2, 0.2, 0.2, 0.2, 0.2, 0.2, 0.2, 0.2,
                    0.2],
                   [0.2, 0.2, 0.2, 0.2, 0.2, 0.2, 0.2, 0.2, 0.2, 0.2, 0.2,
                    0.2]]), 'other_uncert': StdDevUncertainty([[0.1, 0.1, 0.1, 0.1, 0.1, 0.1, 0.1, 0.1, 0.1, 0.1, 0.1,
                    0.1],
                   [0.1, 0.1, 0.1, 0.1, 0.1, 0.1, 0.1, 0.1, 0.1, 0.1, 0.1,
                    0.1],
                   [0.1, 0.1, 0.1, 0.1, 0.1, 0.1, 0.1, 0.1, 0.1, 0.1, 0.1,
                    0.1],
                   [0.1, 0.1, 0.1, 0.1, 0.1, 0.1, 0.1, 0.1, 0.1, 0.1, 0.1,
                    0.1],
                   [0.1, 0.1, 0.1, 0.1, 0.1, 0.1, 0.1, 0.1, 0.1, 0.1, 0.1,
                    0.1],
                   [0.1, 0.1, 0.1, 0.1, 0.1, 0.1, 0.1, 0.1, 0.1, 0.1, 0.1,
                    0.1],
                   [0.1, 0.1, 0.1, 0.1, 0.1, 0.1, 0.1, 0.1, 0.1, 0.1, 0.1,
                    0.1],
                   [0.1, 0.1, 0.1, 0.1, 0.1, 0.1, 0.1, 0.1, 0.1, 0.1, 0.1,
                    0.1],
                   [0.1, 0.1, 0.1, 0.1, 0.1, 0.1, 0.1, 0.1, 0.1, 0.1, 0.1,
                    0.1],
                   [0.1, 0.1, 0.1, 0.1, 0.1, 0.1, 0.1, 0.1, 0.1, 0.1, 0.1,
                    0.1]]), 'result_data': array([[  1.,   2.,   3.,   4.,   5.,   6.,   7.,   8.,   9.,  10.,  11.,
         12.],
       [ 13.,  14.,  15.,  16.,  17.,  18.,  19.,  20.,  21.,  22.,  23.,
         24.],
       [ 25.,  26.,  27.,  28.,  29.,  30.,  31.,  32.,  33.,  34.,  35.,
         36.],
       [ 37.,  38.,  39.,  40.,  41.,  42.,  43.,  44.,  45.,  46.,  47.,
         48.],
       [ 49.,  50.,  51.,  52.,  53.,  54.,  55.,  56.,  57.,  58.,  59.,
         60.],
       [ 61.,  62.,  63.,  64.,  65.,  66.,  67.,  68.,  69.,  70.,  71.,
         72.],
       [ 73.,  74.,  75.,  76.,  77.,  78.,  79.,  80.,  81.,  82.,  83.,
         84.],
       [ 85.,  86.,  87.,  88.,  89.,  90.,  91.,  92.,  93.,  94.,  95.,
       [ 85.,  86.,  87.,  88.,  89.,  90.,  91.,  92.,  93.,  94.,  95.,
         96.],
       [ 97.,  98.,  99., 100., 101., 102., 103., 104., 105., 106., 107.,
       [ 97.,  98.,  99., 100., 101., 102., 103., 104., 105., 106., 107.,
        108.],
        108.],
       [109., 110., 111., 112., 113., 114., 115., 116., 117., 118., 119.,
        120.]]), 'correlation': 0, 'subtract': False, 'to_variance': <ufunc 'square'>, 'from_variance': <ufunc 'sqrt'>, 'correlation_sign': 1, 'result_unit_sq': No       [109., 110., 111., 112., 113., 114., 115., 116., 117., 118., 119.,
        120.]]), 'correlation': 0, 'subtract': False, 'to_variance': <ufunc 'square'>, 'from_variance': <ufunc 'sqrt'>, 'correlation_sign': 1, 'result_unit_sq': None}

Then, when checking how result_unit_sq is used, it shows this:

(Pdb) l
712                 ):
713                     # If the other uncertainty has a unit and this unit differs
714                     # from the unit of the result convert it to the results unit
715                     other = (
716                         to_variance(other_uncert.array << other_uncert.unit)
717  ->                     .to(result_unit_sq)
718                         .value
719                     )
720                 else:
721                     other = to_variance(other_uncert.array)
722             else:

This is not the same as expected, since it entered the conditional scenario when the two objects' units are different, but in the test case written, the two units are set the same.

TODO: 1) look more into the stack trace that pytest --pdb, ll, w, returns. 2) using pytest --pdb, u, check how result_unit_sq is assigned.

2, The test cases testing the case when neither of them has a unit, and that when only the NDData has a unit, have been implemented and passed. TODO: add the test case for when only NDCube has a unit.

@DanRyanIrish DanRyanIrish changed the title Added new features to the ndcube._add_ method Added new features to the ndcube.__add__ method Feb 25, 2025
@DanRyanIrish
Copy link
Member

Here is an example of including fixtures in parameterisations: https://github.com/sunpy/ndcube/blob/main/ndcube/tests/test_ndcube.py#L48
The trick is to add this indirect kwarg at the end of the parameterisation given the name of the input variable(s) that has to be retrieved from conftext.py

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants