sunpy
NDCube.fill method
PR Description
This pull request aims to resolve the issue created by @DanRyanIrish , https://github.com/sunpy/ndcube/issues/828 .
@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? ).
Hi @DanRyanIrish , just so you know, the last three commits are the latest ones I made. Thank you.
Hi @DanRyanIrish , I have finished debugging the tests (with the latest four commits): 1, There were some errors in the fixtures I used, so I fixed them.
2, I forgot to specify the argument "check_uncertainty_values" specifying whether it is necessary to check the uncertainty, as True, in the assert_cubes_equal method. So I fixed it.
3, The coverage file reminded me that I did not test the case when uncertainty_fill_value is not None but the NDCube object's uncertainty is None. So I added an individual test method to test this case, expecting it to raise a TypeError. I wish to ask you a question: Do you think this is the best way to add this test case? I added it by adding an individual method because I am not sure what to put in the expected_cube parametrized argument for this scenario if I were to use the existing two test method structure.
4, Then the coverage file complains about not entering the two scenarios when one and only one of test_input and expected_cube has an uncertainty attribute. But I think the correct behaviour is that it does not enter these two blocks. So I used 'pragma no cover' to let the coverage file know it does not need to be covered by the tests.
Please let me know if there's anything you'd like me to adjust, thank you!
Hi @DanRyanIrish , thanks for your review and bringing up the single-Boolean case issue. In my latest commit, there a few new changes addressing the single-Boolean-True mask value test case, I think the test is more robust now. Please let me know if you have any further feedback, thanks.
I've encountered various astropy masked things recently, they use filled(): https://docs.astropy.org/en/stable/api/astropy.utils.masked.Masked.html maybe we want to consider that?
Both astropy.utils.masked.Masked and numpy.ma.MaskedArray name their analogous method filled(). The distinction I'd draw is that both of those classes are expressly for masked use – the word is in their names! – so including "masked" in a method name would be superfluous. In contrast, I don't feel like masking is at the forefront of all NDCube usage, so I think users need to be alerted that the method is relevant only for masked NDCube objects.
