Deprecate check_broadcast

Blocked by

Broadcast frames over both data and attributes #15121

Description

Numpy 1.20.0 added a function called broadcast_shapes which can replace check_broadcast from astropy.utils. Deprecate check_broadcast.

Expected behavior

N/A

How to Reproduce

N/A

Versions

Astropy 5.3.2

Should this be done in v6.0 or later?

Hello 🙂.
Can you please assign this issue to me. I wish to work on this.

@Shaheer-Ahmd

@Shaheer-Ahmd, I wish you would wait. This is likely to touch a lot of the same code as #15121.

@Shaheer-Ahmd

@Shaheer-Ahmd, thanks for your patience. #15121 was merged. Would you like to take a crack at this issue now?

Hey! Is this issue available? If so, I would like to collaborate as my first issue.

I have not seen any pull request tied to this issue yet, so it is still available.

We do not assign issues to specific people. First come, first serve. And if there are multiple proposed solutions, we pick the best one. Thanks!

Although it is true that our check_broadcast() and broadcast_shapes() from numpy are similar, the errors they raise if the inputs are not compatible are very different, which means that broadcast_shapes() is not a drop-in replacement for check_broadcast() (see the problems #15499 is having). Furthermore, the differences in the errors mean that changing the implementation of check_broadcast() to make use of broadcast_shapes() would not be a simplification. It doesn't look like check_broadcast() can be deprecated any time soon.

Thanks for investigating. Should we close this as "won't fix" then?

Should we close this as "won't fix" then?

Before we go that far, I would like to take a closer look at this. Remember that there are some places where check_broadcast isn't actually doing anything due to a bug.

Although it is true that our check_broadcast() and broadcast_shapes() from numpy are similar, the errors they raise if the inputs are not compatible are very different, which means that broadcast_shapes() is not a drop-in replacement for check_broadcast() (see the problems #15499 is having). Furthermore, the differences in the errors mean that changing the implementation of check_broadcast() to make use of broadcast_shapes() would not be a simplification. It doesn't look like check_broadcast() can be deprecated any time soon.

When np.broadcast_shapes() fails due to incompatible shapes, it raises a ValueError that explains which arguments failed to broadcast and what their shapes were:

>>> np.broadcast_shapes((3,), (4,))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/lpsinger/Library/Python/3.11/lib/python/site-packages/numpy/lib/stride_tricks.py", line 473, in broadcast_shapes
    return _broadcast_shape(*arrays)
           ^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/lpsinger/Library/Python/3.11/lib/python/site-packages/numpy/lib/stride_tricks.py", line 422, in _broadcast_shape
    b = np.broadcast(*args[:32])
        ^^^^^^^^^^^^^^^^^^^^^^^^
ValueError: shape mismatch: objects cannot be broadcast to a single shape.  Mismatch is between arg 0 with shape (3,) and arg 1 with shape (4,).

This error message contains all of the same information that is present now in IncompatibleShapeError, although not in a machine-friendly format as separate exception attributes. However there is only one place in Astropy where that information is actually used, in astropy.modeling:

astropy/astropy/modeling/core.py

Lines 2745 to 2755 in 05f7b2c

    
           except IncompatibleShapeError as exc: 
        
               shape_a, shape_a_idx, shape_b, shape_b_idx = exc.args 
        
               param_a = self.param_names[shape_a_idx] 
        
               param_b = self.param_names[shape_b_idx] 
        
               raise InputParameterError( 
        
                   f"Parameter {param_a!r} of shape {shape_a!r} cannot be broadcast with " 
        
                   f"parameter {param_b!r} of shape {shape_b!r}.  All parameter arrays " 
        
                   "must have shapes that are mutually compatible according " 
        
                   "to the broadcasting rules." 
        
               )

Is keeping check_broadcast worth it in order to preserve that pretty-printed exception?

@WilliamJamieson

there is only one place in Astropy where that information is actually used, in astropy.modeling

modeling is a tricky beast and helpful error message would make user less frustrated when their modeling code fails. So to make the error message less informative would be undesirable but @WilliamJamieson can comment.

@lpsinger

I think I'm with @lpsinger here, in that I think ideally we do deprecate check_broadcast -- we would never have introduced it if np.broadcast_shapes had existed, and it seems needless duplication. That said, I do think we should minimize changes, i.e., we should keep the pretty-printed exception - it would not seem particularly tricky to write a regex that gets the same information out of the exception message.

...it would not seem particularly tricky to write a regex that gets the same information out of the exception message.

We could submit a PR to Numpy to add this data to the exception as a tuple.

@lpsinger

I think I'm with @lpsinger here, in that I think ideally we do deprecate check_broadcast ...

Alright. Deprecating the function is easy. Do we replace uses of it inside Astropy with the Numpy function immediately, or would that be an API change because we would be replacing IncompatibleShapeError with ValueError?

Deprecating should happen at the same time as replacing it in the codebase. We do not want to use the thing that we have deprecated ourselves.

We could submit a PR to Numpy to add this data to the exception as a tuple.

I quite like that idea (but won't have time to do it...). If would then make sense to also try to make the error type more specific.

I'm not sure how feasible it is to upstream to numpy: it seems to me that it would require storing data on the exception object in a new (supposedly private) attribute, which is unusual and seems fragile ?

For reference, the exception is raised from C-API, which I think makes it even less likely that such a change would be accepted
https://github.com/numpy/numpy/blob/2da02ea321f557c0cfe0ad6d0e7d8a4354c51103/numpy/_core/src/multiarray/iterators.c#L1213-L1215

All things considered, I now think that using a regexp in modeling.core to rehash the exception raised by numpy might actually be less fragile. What do you guys think ?

All things considered, I now think that using a regexp in modeling.core to rehash the exception raised by numpy might actually be less fragile. What do you guys think ?

Seems the easiest option indeed - so, that means only in modelling would there be a bit of code around broadcast_shapes that prettifies the exception message, right?

I think so. I'll get right on it.

Navigation Menu

Repository navigation

Deprecate check_broadcast #15211

Blocked by

Description

Expected behavior

How to Reproduce

Versions

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	except IncompatibleShapeError as exc:
	shape_a, shape_a_idx, shape_b, shape_b_idx = exc.args
	param_a = self.param_names[shape_a_idx]
	param_b = self.param_names[shape_b_idx]

	raise InputParameterError(
	f"Parameter {param_a!r} of shape {shape_a!r} cannot be broadcast with "
	f"parameter {param_b!r} of shape {shape_b!r}. All parameter arrays "
	"must have shapes that are mutually compatible according "
	"to the broadcasting rules."
	)

Deprecate check_broadcast #15211

Description

Blocked by

Description

Expected behavior

How to Reproduce

Versions

Activity

pllim commented on Aug 21, 2023

Shaheer-Ahmd commented on Aug 22, 2023

lpsinger commented on Aug 22, 2023

lpsinger commented on Oct 3, 2023

felipekhouri commented on Oct 5, 2023

pllim commented on Oct 5, 2023

eerovaher commented on Nov 1, 2023

pllim commented on Nov 1, 2023

lpsinger commented on Nov 1, 2023

lpsinger commented on Nov 1, 2023

pllim commented on Nov 1, 2023

mhvk commented on Nov 2, 2023

lpsinger commented on Nov 2, 2023

lpsinger commented on Nov 2, 2023

pllim commented on Nov 2, 2023

mhvk commented on Nov 2, 2023

neutrinoceros commented on Apr 25, 2024

mhvk commented on Apr 25, 2024

neutrinoceros commented on Apr 25, 2024

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions