Fixed a bug in NDCubeSequence.cube_like_dimensions.

[PCJY]

PR Description

fixes issue #785

[PCJY]

Hi @DanRyanIrish Here is my first Pull Request, can you please review it?

[nabobalis]

Hello @PCJY, does this happen on a release version of ndcube or just the main version?

It might be useful to also add a unit test to check we don't break this in the future.

[DanRyanIrish]

Hi @PCJY. Thanks so much for this PR. I agree with @nabobalis that we should also have a unit test for this. In addition, you should add a changelog file to describe the change that this PR makes.

For an example of a unit test, you can see here. It tests NDCubeSequence.cube_like_shape, so can be very closely adapted to this PR. Your test_cube_like_dimensions test should go in the same file, after the test_cube_like_shape test function.

For a quick explainer of changelog files, see here. You should add the file to the ndcube/changelog directory.

[nabobalis]

Does this change also need to occur to the new property as well?

[DanRyanIrish]

Does this change also need to occur to the new property as well?

Which new property @nabobalis ?

[nabobalis]

Does this change also need to occur to the new property as well?

Which new property @nabobalis ?

warn_deprecated("Replaced by ndcube.NDCubeSequence.cube_like_shape") ? Or is the new one unitless?

[DanRyanIrish]

warn_deprecated("Replaced by ndcube.NDCubeSequence.cube_like_shape") ? Or is the new one unitless?

Yes, the new one is unitless.

[DanRyanIrish]

Hi @PCJY Thanks for these changes. Currently, the test is failing because of the deprecation warning that this function raises. Therefore, we have to make this test ignore such warnings. @nabobalis, do you know how to ignore deprecation warnings for a specific test?

[DanRyanIrish]

To avoid the deprecation warning making this test fail, I think you should be able to use Python's catch_warnings context manager

[DanRyanIrish]

This looks like a perfectly good solution. The issue before was that assert_quantity_allclose returns None if the values are the same, not true. So an alternative fix would have been

Suggested change

	for ndc_dim, exp_dim in zip(ndc.cube_like_dimensions, expected_dimensions):
	assert_quantity_allclose(ndc_dim, exp_dim)
	assert all(assert_quantity_allclose(ndc_dim, exp_dim) is None for ndc_dim, exp_dim in zip(ndc.cube_like_dimensions, expected_dimensions))

But if this works, we can keep your solution :)

[DanRyanIrish]

The pre-commit test failed. You should run tox -e codestyle locally an then commit and push the changes.

In addition, the "This branch is out-of-date with the base branch" warning below the tests means the new updates have been made to the main branch. So in addition, yout should do git pull origin main locally and then push the changes, so the PR is up-to-date.

I'll look into the docs test fail later.

[nabobalis]

Doc failure is <unknown>:37: WARNING: py:obj reference target not found: NDCubeSequence.cube_like_dimensions [ref.obj]

[DanRyanIrish]

Doc failure is <unknown>:37: WARNING: py:obj reference target not found: NDCubeSequence.cube_like_dimensions [ref.obj]

Thanks @nabobalis. Do you know what that means?

[nabobalis]

Doc failure is <unknown>:37: WARNING: py:obj reference target not found: NDCubeSequence.cube_like_dimensions [ref.obj]

Thanks @nabobalis. Do you know what that means?

Yes I do.

[DanRyanIrish]

Congrats @PCJY on your first approved PR to ndcube!!