NDCube.fill method #829
NDCube.fill method #829
Conversation
There was a problem hiding this comment.
Hi @PCJY. Here are some suggested improvements.
Once you've handled these, you should also consider how to handle the case where the cube (including the uncertainty) has a unit.
Then this PR will also need tests.
Co-authored-by: DanRyanIrish <[email protected]>
rename the method Co-authored-by: DanRyanIrish <[email protected]>
Co-authored-by: DanRyanIrish <[email protected]>
Co-authored-by: DanRyanIrish <[email protected]>
remove code about kwargs when fill_in_place is True. Co-authored-by: DanRyanIrish <[email protected]>
Already know that self.mask is not None, now, check whether uncertainty_fill_value and self.uncertainty is None, raise an error if self.uncertainty is None. Co-authored-by: DanRyanIrish <[email protected]>
Filling in the uncertainty_fill_value Co-authored-by: DanRyanIrish <[email protected]>
Co-authored-by: DanRyanIrish <[email protected]>
Co-authored-by: DanRyanIrish <[email protected]>
|
@PCJY Let me know when this is ready for another review. Also, be sure to pull the latest changes from the main branch into this branch. |
|
Hi @DanRyanIrish I have just implemented the method more, finished adding tests for the method. Apologies for the delay. I also handled units by keeping the unit the same (which might not be what you were trying to suggest, because you might mean: the fill_value itself has a unit? ). |
ndcube/ndcube.py
Outdated
| idx_mask = slice(None) is self.mask is True else self.mask # Ensure indexing mask can index the data array. | ||
| kwargs["data"][self.mask] = fill_value # Boolean indexing in Python. |
There was a problem hiding this comment.
| idx_mask = slice(None) is self.mask is True else self.mask # Ensure indexing mask can index the data array. | |
| kwargs["data"][self.mask] = fill_value # Boolean indexing in Python. | |
| idx_mask = slice(None) is self.mask is True else self.mask # Ensure indexing mask can index the data array. | |
| new_data[idx_mask] = fill_value # Boolean indexing in Python. |
| (1.0 * u.cm, 0.02, False, False), # what if the fill_value has a unit?? | ||
| ] | ||
| ) | ||
| def test_fill_masked(ndcube_2d_ln_lt_mask_uncert_unit, fill_value, uncertainty_fill_value, unmask, fill_in_place): |
There was a problem hiding this comment.
I suggest two separate tests for fill_in_place equal True and False. I also suggest that you provide the expected cube in the parameterisation. This would be much easier if your test cube only had a few data elements, e.g. a 2x3 array.
Where it's feasible, it's better to explicitly define the expected value, rather than calculate it based on the inputs provided. This leaves more possibility for the same error to exist in the code you're testing and your calculation of the expected value.
| new_data = self.data | ||
| new_uncertainty = self.uncertainty | ||
| new_mask = False if unmask else self.mask | ||
| new_unit = self.unit |
There was a problem hiding this comment.
The unit should stay the same, so doesn't need to be set here or in the else statement below. Instead, I meant that, after this if...else statement, you should check whether fill_value has a unit and then convert it to the same unit as the cube, as then strip the unit, i.e.
if hasattr(fill_value, "unit")`:
fill_value = fill_value.to_value(self.unit)
You might need to do something equivalent for uncertainty_fill_value?
make kwargs ndcube and return it Co-authored-by: DanRyanIrish <[email protected]>
Co-authored-by: DanRyanIrish <[email protected]>
what's needed to be done regarding units is checking whether units assigned are consistent, if users do assign it Co-authored-by: DanRyanIrish <[email protected]>
Co-authored-by: DanRyanIrish <[email protected]>
PR Description
This pull request aims to resolve the issue created by @DanRyanIrish , #828 .