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 12 commits into
base: main
Choose a base branch
from
32 changes: 32 additions & 0 deletions ndcube/ndcube.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

import astropy.nddata
import astropy.units as u
from astropy.nddata import NDData
from astropy.units import UnitsError

try:
Expand Down Expand Up @@ -918,6 +919,37 @@
return self._new_instance(data=-self.data)

def __add__(self, value):
if isinstance(value, NDData) and value.wcs is None:
if self.unit is not None and value.unit is not None:
value_data = value.data * value.unit.to(self.unit)
PCJY marked this conversation as resolved.
Show resolved Hide resolved
elif self.unit is None:
value_data = value.data

Check warning on line 926 in ndcube/ndcube.py

View check run for this annotation

Codecov / codecov/patch

ndcube/ndcube.py#L923-L926

Added lines #L923 - L926 were not covered by tests
else:
raise TypeError("Cannot add unitless NDData to a unitful NDCube.")

Check warning on line 928 in ndcube/ndcube.py

View check run for this annotation

Codecov / codecov/patch

ndcube/ndcube.py#L928

Added line #L928 was not covered by tests

# addition
new_data = self.data + value_data

Check warning on line 931 in ndcube/ndcube.py

View check run for this annotation

Codecov / codecov/patch

ndcube/ndcube.py#L931

Added line #L931 was not covered by tests
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.

# combine the uncertainty
new_uncertainty = None
if self.uncertainty is not None and value.uncertainty is not None:
new_uncertainty = self.uncertainty.propagate(

Check warning on line 935 in ndcube/ndcube.py

View check run for this annotation

Codecov / codecov/patch

ndcube/ndcube.py#L933-L935

Added lines #L933 - L935 were not covered by tests
np.add, value.uncertainty, correlation=0
)
elif self.uncertainty is not None:
new_uncertainty = self.uncertainty
elif value.uncertainty is not None:
new_uncertainty = value.uncertainty

Check warning on line 941 in ndcube/ndcube.py

View check run for this annotation

Codecov / codecov/patch

ndcube/ndcube.py#L938-L941

Added lines #L938 - L941 were not covered by tests
PCJY marked this conversation as resolved.
Show resolved Hide resolved

# combine mask
self_ma = np.ma.MaskedArray(self.data, mask=self.mask)
value_ma = np.ma.MaskedArray(value_data, mask=value.mask)
result_ma = self_ma + value_ma
new_mask = result_ma.mask

Check warning on line 947 in ndcube/ndcube.py

View check run for this annotation

Codecov / codecov/patch

ndcube/ndcube.py#L944-L947

Added lines #L944 - L947 were not covered by tests

# return the new NDCube instance
return self._new_instance(

Check warning on line 950 in ndcube/ndcube.py

View check run for this annotation

Codecov / codecov/patch

ndcube/ndcube.py#L950

Added line #L950 was not covered by tests
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

if hasattr(value, 'unit'):
if isinstance(value, u.Quantity):
# NOTE: if the cube does not have units, we cannot
Expand Down
Loading