NDCube.fill_masked() method

[PCJY]

PR Description

This pull request aims to resolve the issue proposed by @DanRyanIrish , #828 .

[DanRyanIrish]

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.

[DanRyanIrish]

@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.

[PCJY]

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? ).

[DanRyanIrish]

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.

[PCJY]

Hi @DanRyanIrish , just so you know, the last three commits are the latest ones I made. Thank you.

[PCJY]

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!

[DanRyanIrish]

Hi @PCJY. This is looking good. Just one comment about testing masks are equal.

I'd like to get @Cadair to give this a quick look if possible. But I think it's close to being done.

[DanRyanIrish]

@Cadair Do you have advice when it comes to methods to make changes in-place? Should this return None, or not return anything at all?

[PCJY]

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.

[Cadair]

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?

[ayshih]

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.