WIP: fix input broadcasting in modeling#10362
Conversation
|
It would be good to give an example of That is, point to such a model that does that. |
Does it only happen during full moon? :D |
perrygreenfield
left a comment
There was a problem hiding this comment.
A few comments, but otherwise looks good.
| if broadcasted_shape is not None: | ||
| # Ensure parameter and input shapes broadcast | ||
| broadcasted_shape = check_broadcast(broadcasted_shape, param_broadcast) | ||
| shapes_of_inputs = [check_broadcast(shape, param_broadcast) for shape in shapes_of_inputs] |
There was a problem hiding this comment.
I am wondering of this check of broadcast should be using broadcasted_shape instead of param_broadcast, in which case an empty broadcasted_shape may need modification if check_broadcast can not handle it.
| # first check inputs are consistent in shape | ||
| input_shape = _validate_input_shapes(args, (), self._n_models, | ||
| self.model_set_axis, self.standard_broadcasting) | ||
| self.model_set_axis) |
There was a problem hiding this comment.
Will this cause problems if self.standard_broadcasting is False (well, it looks like this doesn't change past behavior, but still, is it not a problem?)
| result = c(1, [1, 1]) | ||
| expected = [1., 0., [2., 2.]] | ||
| for r, e in zip (result, expected): | ||
| assert_allclose(r, e) |
There was a problem hiding this comment.
Need a test case where inputs are not shape compatible for a standard_broadcasting=False
Added tests to show that issue astropy#10170 is fixed. Added `towncrier` change log. Fixed codestyle check by adding specific exceptions to check. Possible fixes for issue astropy#11360. More investigation is necessary. Cleaned up last bugfix. Updated change log entry. Added tests for astropy#11360 fixes. Fixed whitespace Fix for jwst and gwcs test failures. Added testing for issue causing fails in jwst and gwcs Fixed a typo. Added some of astropy#10362 tests Changes in astropy#10362 to pass new tests Code cleanup More code cleanup Added additional testing from astropy#10362 Possible fix for issue astropy#11060. Added test to show astropy#11060 has been fixed Fixed failing test under no-scipy Added future warning ignore to newest test Modified tabular so that both inputs in issue astropy#11060 produce same output
This PR is functionally complete but I marked it as WIP to get opinions on the approach. I will add documentation later.
This takes a slightly different approach to validating inputs in modeling.
The previous behavior was:
Model.standard_broadcasting=Trueparameters and inputs were broadcasted to a common shape.The new approach:
Model.standard_broadcasting=Truemeans that broadcasting rules apply to inputs and parameters.prepare_inputsis not broadcasting the inputs before passing them toModel.evaluate(although it computes the broadcasted shape ) but let's the function implementation deal with this following numpy broadcasting rules.Model.standard_broadcasting=Falseit is not assumed that inputs should broadcast nor that inputs and parameters should broadcast. Dealing with input and output shapes is left to the writer of the model. This is a rare use case but is necessary.n_outputs > n_inputswithstandard_broadcasting=True. In this case the shape fo the extra outputs is assumed to be the precomputed broadcasted shape.Fixes #10170
Fixes #9953