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 24 commits into
base: main
Choose a base branch
from
Open
Changes from 5 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
9c38077
Added new features to the ndcube._add_ method
PCJY Dec 11, 2024
1d5d2ab
Merge branch 'main' of https://github.com/sunpy/ndcube into nddataAri…
PCJY Dec 16, 2024
aaa9ef0
Update ndcube/ndcube.py
PCJY Dec 16, 2024
ed4f61e
Update ndcube/ndcube.py
PCJY Dec 16, 2024
ea43a1d
Modified the _add_ method further.
PCJY Dec 18, 2024
a891ff9
Merge branch 'nddataArithmetic' of https://github.com/PCJY/ndcube int…
PCJY Dec 18, 2024
f575e2c
Further modifies the _add_ method.
PCJY Dec 23, 2024
8951635
Added a changelog file for this new feature.
PCJY Dec 23, 2024
58e4363
Merge branch 'main' of https://github.com/sunpy/ndcube into nddataAri…
PCJY Dec 23, 2024
bcf4fb9
Added a new method test_cube_add_uncertainty_and_mask to test_ndcube.py.
PCJY Dec 23, 2024
c4d639a
Modified the test_cube_add_uncertainty_and_mask method in test_ndcube.py
PCJY Dec 23, 2024
bd317e3
Modified the test_cube_add_uncertainty_and_mask further.
PCJY Dec 23, 2024
e0375ec
Fixed how the masks are combined.
PCJY Jan 14, 2025
0158737
Set masked uncertainty entries to 0.
PCJY Jan 18, 2025
9074f45
Moved uncertainty combination out of the mask-combining If Statements.
PCJY Jan 21, 2025
9e267d3
Merge branch 'main' of https://github.com/sunpy/ndcube into nddataAri…
PCJY Jan 21, 2025
d8c2db9
Merge branch 'main' into nddataArithmetic
PCJY Jan 21, 2025
5f422f5
Removed mask-dealing in the add method.
PCJY Jan 22, 2025
3369223
Merge branch 'nddataArithmetic' of https://github.com/PCJY/ndcube int…
PCJY Jan 22, 2025
f17da78
Removed mask-dealing in the Add method.
PCJY Jan 22, 2025
344b6f7
use a conditional statement to still check whether there is a mask.
PCJY Jan 22, 2025
7ff78aa
Changed mask to False and removed mask-checking in test_cube_add_unce…
PCJY Jan 22, 2025
5852daa
Added placeholders for using the new parameters and modified the no-m…
PCJY Jan 22, 2025
5dcb8ff
Set default of operation_ignores_mask to be True.
PCJY Jan 22, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 15 additions & 9 deletions ndcube/ndcube.py
Original file line number Diff line number Diff line change
Expand Up @@ -1035,37 +1035,43 @@
return self._new_instance(data=-self.data)

def __add__(self, value):
kwargs = {}
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)
value_data = (value.data * value.unit).to_value(self.unit)
elif self.unit is None:
value_data = value.data

Check warning on line 1043 in ndcube/ndcube.py

View check run for this annotation

Codecov / codecov/patch

ndcube/ndcube.py#L1040-L1043

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

Check warning on line 1045 in ndcube/ndcube.py

View check run for this annotation

Codecov / codecov/patch

ndcube/ndcube.py#L1045

Added line #L1045 was not covered by tests

# addition
new_data = self.data + value_data
# 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 1049 in ndcube/ndcube.py

View check run for this annotation

Codecov / codecov/patch

ndcube/ndcube.py#L1048-L1049

Added lines #L1048 - L1049 were not covered by tests
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

)
kwargs["uncertainty"] = new_uncertainty
elif self.uncertainty is not None:
new_uncertainty = self.uncertainty
kwargs["uncertainty"] = new_uncertainty
elif value.uncertainty is not None:
new_uncertainty = value.uncertainty

Check warning on line 1057 in ndcube/ndcube.py

View check run for this annotation

Codecov / codecov/patch

ndcube/ndcube.py#L1052-L1057

Added lines #L1052 - L1057 were not covered by tests
else:
new_uncertainty = None

Check warning on line 1059 in ndcube/ndcube.py

View check run for this annotation

Codecov / codecov/patch

ndcube/ndcube.py#L1059

Added line #L1059 was not covered by tests

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

Check warning on line 1063 in ndcube/ndcube.py

View check run for this annotation

Codecov / codecov/patch

ndcube/ndcube.py#L1062-L1063

Added lines #L1062 - L1063 were not covered by tests

# addition
result_ma = self_ma + value_ma

Check warning on line 1066 in ndcube/ndcube.py

View check run for this annotation

Codecov / codecov/patch

ndcube/ndcube.py#L1066

Added line #L1066 was not covered by tests
new_mask = result_ma.mask

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

Check warning on line 1070 in ndcube/ndcube.py

View check run for this annotation

Codecov / codecov/patch

ndcube/ndcube.py#L1069-L1070

Added lines #L1069 - L1070 were 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.

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.


# return the new NDCube instance
return self._new_instance(
data=new_data, uncertainty=new_uncertainty, mask=new_mask
)
return self._new_instance(**kwargs)

Check warning on line 1073 in ndcube/ndcube.py

View check run for this annotation

Codecov / codecov/patch

ndcube/ndcube.py#L1073

Added line #L1073 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.

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)


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