API: check_broadcast cannot return None

[lpsinger]

Description

_validate_input_shapes calls check_broadcast and compares its return value with None, but check_broadcast (as far as I can tell) cannot return None.

• By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

[github-actions]

Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.

• Do the proposed changes actually accomplish desired goals?
• Do the proposed changes follow the Astropy coding guidelines?
• Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
• Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
• Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
• Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
• Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
• Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
• Is a milestone set? Milestone must be set but we cannot check for it on Actions; do not let the green checkmark fool you.
• At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

[pllim]

This looks like a bug. If that is the case, then it needs a change log entry. And a regression test (why didn't we see this failing CI?). But let's wait to hear back from modeling maintainers first.

[WilliamJamieson]

This is definitely an oversight as this is handled this way in several other places.. Thanks for finding it.

[lpsinger]

You know, I realized that IncompatibleShapeError is a more useful error message than ValueError. So I just removed the re-raise entirely. This is also consistent with the old behavior, whereas re-raising it as a ValueError would have actually been an API change (since the is None code path was never followed, and check_broadcast simply would have raised).

By the above reasoning, this change has no user-visible impact. So I'm going to change this to MNT.

[lpsinger]

By the above reasoning, this change has no user-visible impact. So I'm going to change this to MNT.

@pllim, if you agree with my assessment, would you please update the labels?

[WilliamJamieson]

This test:

astropy/astropy/modeling/tests/test_core.py

Lines 984 to 1004 in dce55cd

	def test__validate_input_shapes():
	model = models.Gaussian1D()
	model._n_models = 2
	inputs = [mk.MagicMock() for _ in range(3)]
	argnames = mk.MagicMock()
	model_set_axis = mk.MagicMock()
	all_shapes = [mk.MagicMock() for _ in inputs]
	# Successful validation
	with mk.patch.object(
	Model, "_validate_input_shape", autospec=True, side_effect=all_shapes
	) as mkValidate:
	with mk.patch.object(core, "check_broadcast", autospec=True) as mkCheck:
	assert mkCheck.return_value == model._validate_input_shapes(
	inputs, argnames, model_set_axis
	)
	assert mkCheck.call_args_list == [mk.call(*all_shapes)]
	assert mkValidate.call_args_list == [
	mk.call(model, _input, idx, argnames, model_set_axis, True)
	for idx, _input in enumerate(inputs)
	]

needs to be refactored to:

def test__validate_input_shapes():
    model = models.Gaussian1D()
    model._n_models = 2


    # Full success
    inputs = [np.array([[1, 2], [3, 4]]), np.array([[5, 6], [7, 8]])]
    assert (2, 2) == model._validate_input_shapes(inputs, model.inputs, 1)

    # Fail check_broadcast
    MESSAGE = r"All inputs must have identical shapes or must be scalars"

    # Fails because the input shape of the second input has one more axis which
    # for which the first input can be broadcasted to
    inputs = [np.array([[1, 2], [3, 4]]), np.array([[5, 6], [7, 8], [9, 10]])]
    with pytest.raises(ValueError, match=MESSAGE):
        model._validate_input_shapes(inputs, model.inputs, 1)

(this will need to be updated to catch the IncompatibleShapeError)

[WilliamJamieson]

You know, I realized that IncompatibleShapeError is a more useful error message than ValueError. So I just removed the re-raise entirely. This is also consistent with the old behavior, whereas re-raising it as a ValueError would have actually been an API change (since the is None code path was never followed, and check_broadcast simply would have raised).

By the above reasoning, this change has no user-visible impact. So I'm going to change this to MNT.

I agree.

[WilliamJamieson]

After examining the IncompatableShapeError I think I like previous error message better.

For example the test_core test which was refactored gives the message:

astropy.utils.shapes.IncompatibleShapeError: ((2, 2), 0, (3, 2), 1)

which is a bit opaque to as to what the issue is.

[lpsinger]

Alright. How about if I raise a new IncompatibleShapeError but with a better explanation?

[WilliamJamieson]

The current message is actually pretty clear about what users should be doing.

[lpsinger]

OK, got it. That will make this an API change, though, since IncompatibleShapeError doesn't support custom error messages, but ValueError does. Sorry, @pllim!

[pllim]

No worries. Let me know when to update the labels again. 😆 API change needs a change log for sure.

[pllim]

would you please update the labels?

Can you please be more specific on which label needs updating? Thanks!

[pllim]

Re: labels -- Ah, you mean the change log? If you are changing the exception type that gets raised, it still needs a change log. And as stated above, I don't think we should backport that because that is strictly not backward compatible.

[lpsinger]

would you please update the labels?

Can you please be more specific on which label needs updating? Thanks!

I mean, remove the Bug label. This should be treated as a MNT-only change. So, no need for a changelog entry.

[pllim]

I mean, remove the Bug label

So no backport, right? I also will move the milestone to v6 then, ok?

[pllim]

no need for a changelog entry

Depends. Are you really changing the exception type or not?

[lpsinger]

I mean, remove the Bug label

So no backport, right? I also will move the milestone to v6 then, ok?

Correct, no backport, and v6 milestone is fine.

[lpsinger]

no need for a changelog entry

Depends. Are you really changing the exception type or not?

I am not changing the exception type. We currently raise an IncompatibleShapeError, and we will still raise an IncompatibleShapeError. (Read #15209 (comment) carefully.)

[pllim]

LGTM now, thanks! But I'll let William approve.