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 <ryand5@tcd.ie>
rename the method Co-authored-by: DanRyanIrish <ryand5@tcd.ie>
Co-authored-by: DanRyanIrish <ryand5@tcd.ie>
Co-authored-by: DanRyanIrish <ryand5@tcd.ie>
remove code about kwargs when fill_in_place is True. Co-authored-by: DanRyanIrish <ryand5@tcd.ie>
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 <ryand5@tcd.ie>
Filling in the uncertainty_fill_value Co-authored-by: DanRyanIrish <ryand5@tcd.ie>
Co-authored-by: DanRyanIrish <ryand5@tcd.ie>
Co-authored-by: DanRyanIrish <ryand5@tcd.ie>
|
@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/tests/test_ndcube.py
Outdated
| (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.
make kwargs ndcube and return it Co-authored-by: DanRyanIrish <ryand5@tcd.ie>
Co-authored-by: DanRyanIrish <ryand5@tcd.ie>
what's needed to be done regarding units is checking whether units assigned are consistent, if users do assign it Co-authored-by: DanRyanIrish <ryand5@tcd.ie>
Co-authored-by: DanRyanIrish <ryand5@tcd.ie>
Co-authored-by: DanRyanIrish <ryand5@tcd.ie>
|
Hi @DanRyanIrish , just so you know, the last three commits are the latest ones I made. Thank you. |
Co-authored-by: DanRyanIrish <ryand5@tcd.ie>
PR Description
This pull request aims to resolve the issue created by @DanRyanIrish , #828 .