Bugfixes for `Model` crashes when some inputs are scalars. by WilliamJamieson · Pull Request #11548 · astropy/astropy

WilliamJamieson · 2021-04-13T15:35:44Z

Description

This provides bugfixes for model crashes when some inputs are scalars (or smaller array which can broadcast to larger array). The main issue occurs when an earlier (in input order) input is a smaller array which must be broadcast to be compatible with a larger array. This issue occurs because the output should be the shape of the larger array, but _prepare_outputs_single_model attempts to reshape the array to fit into the first input's shape. In reality, the outputs should be reshaped to the broadcast shape of all the input arrays. This is what has been implemented here, along side tests verifying that the issue has been fixed.

Also adds fixes to allow outputs to be correctly reshaped without crashing in other circumstances.

Fixes #10170, Fixes #11360, Fixes #9953, and Fixes #11060.
Closes #10362

pep8speaks · 2021-04-13T15:35:56Z

Hello @WilliamJamieson 👋! It looks like you've made some changes in your pull request, so I've checked the code again for style.

There are no PEP8 style issues with this pull request - thanks! 🎉

Comment last updated at 2021-04-27 14:47:34 UTC

pllim · 2021-04-13T15:36:59Z

Does this also fix #9953 ?

pllim · 2021-04-13T15:37:59Z

Does this also supersede #10362 ?

WilliamJamieson · 2021-04-13T18:14:41Z

Tagging @mcara.

WilliamJamieson · 2021-04-13T20:17:29Z

Does this also fix #9953 ?

This does not fix #9953.

Does this also supersede #10362 ?

Due to the above note, I do not know if it supersedes #10362.

mcara · 2021-04-14T07:50:10Z

All I can say is that I am happy my example, which was previously crashing, is now passing unit tests. So it seems to fix my issue.

Thanks @WilliamJamieson !

WilliamJamieson · 2021-04-15T16:04:59Z

Does this also fix #9953 ?

This does not fix #9953.

Does this also supersede #10362 ?

Due to the above note, I do not know if it supersedes #10362.

I have made the changes to fix #9953 and so this now supersedes #10362.

WilliamJamieson · 2021-04-19T19:46:14Z

I believe I may have a solution to issue #11060. If the solution presented in this comment seems to fix things, I can include that solution as part of this PR.

WilliamJamieson · 2021-04-22T17:56:30Z

I believe I may have a solution to issue #11060. If the solution presented in this comment seems to fix things, I can include that solution as part of this PR.

Additional testing has shown that this fix is necessary.

perrygreenfield

LGTM. When reviewing prepare_inputs and prepare_outputs variants I always have trouble following what the products of one are. For example, I would not guess that format_info actually is related to broadcast shapes, and I always waste time figuring that stuff out again. At some point in the future some better outline of explaining how all that works ought to help make it easier to make changes and review code.

WilliamJamieson · 2021-04-23T19:28:35Z

LGTM. When reviewing prepare_inputs and prepare_outputs variants I always have trouble following what the products of one are. For example, I would not guess that format_info actually is related to broadcast shapes, and I always waste time figuring that stuff out again. At some point in the future some better outline of explaining how all that works ought to help make it easier to make changes and review code.

I agree, at some point we need to refactor so that the shaping that is going on in prepare_inputs and prepare_outputs is more clear.

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

nden

lgtm

chris-simpson · 2021-08-06T00:58:05Z

In reality, the outputs should be reshaped to the broadcast shape of all the input arrays.

Sorry to have come very late to this, but I think this statement is wrong but, more importantly, it prevents fix_inputs working as expected. This PR is listed as fixing #10170 (which I posted), which it does only in a limited set of circumstances. In that issue, I commented that a Gaussian2D model (which has 2 inputs and 1 output) worked if the first input was a sequence and the second a scalar, but not if the first input was a scalar and the second a sequence. This PR now does what it expected for both cases so that g([x,x], y) and g(x, [y,y]) and g([x,x], [y,y]) all return the same thing. But that is not generally true.

Consider the model

>>> m = models.Rotation2D(90.)
>>> m(0,1)
(-1.0, 6.123233995736766e-17)
>>> m([0,1], [1,1])
(array([-1., -1.]), array([6.123234e-17, 1.000000e+00]))
>>> m([0,1], 1)  # should be same as above
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/opt/anaconda3/envs/dev/lib/python3.7/site-packages/astropy/modeling/core.py", line 399, in __call__
    __call__, args, kwargs, varargs='inputs', varkwargs='new_inputs')
  File "/opt/anaconda3/envs/dev/lib/python3.7/site-packages/astropy/modeling/core.py", line 377, in __call__
    return super(cls, self).__call__(*inputs, **kwargs)
  File "/opt/anaconda3/envs/dev/lib/python3.7/site-packages/astropy/modeling/core.py", line 918, in __call__
    return generic_call(self, *new_args, **kwargs)
  File "/opt/anaconda3/envs/dev/lib/python3.7/site-packages/astropy/modeling/core.py", line 4150, in generic_call
    outputs = self.evaluate(*chain(inputs, parameters))
  File "/opt/anaconda3/envs/dev/lib/python3.7/site-packages/astropy/modeling/rotations.py", line 482, in evaluate
    raise ValueError("Expected input arrays to have the same shape")
ValueError: Expected input arrays to have the same shape

The problem here is that it means fix_inputs is still broken, as I cannot do:

>>> m = models.Rotation2D(90.)
>>> m2 = fix_inputs(m, {1: 1})
>>> m2(0)
(-1.0, 6.123233995736766e-17)
>>> m2([0,0])
Traceback (most recent call last):
(same as above)
ValueError: Expected input arrays to have the same shape

but I can do

>>> m2 = fix_inputs(m, {1: [1, 1]})
>>> m2([0,0])
(array([-1., -1.]), array([6.123234e-17, 6.123234e-17]))
>>> m2(0)
Traceback (most recent call last):
(same as above)
ValueError: Expected input arrays to have the same shape

Clearly this is wrong.

pllim · 2021-08-06T14:11:56Z

Oh, no. Do we have to revert this patch?

pllim · 2021-08-06T14:14:10Z

Let's discuss at #12021

github-actions Bot added the modeling label Apr 13, 2021

pllim requested a review from nden April 13, 2021 15:38

pllim added the Bug label Apr 13, 2021

pllim added this to the v4.2.2 milestone Apr 13, 2021

pllim requested a review from perrygreenfield April 13, 2021 15:39

pllim requested a review from mcara April 13, 2021 18:16

This was referenced Apr 15, 2021

Inputs to modeling not always broadcasted as expected #11060

Closed

Fixes bug in LevMarLSQFitter weights. #11603

Merged

perrygreenfield approved these changes Apr 23, 2021

View reviewed changes

WilliamJamieson mentioned this pull request Apr 23, 2021

Add fittable 1D spline models #11634

Merged

WilliamJamieson force-pushed the tickets/AL-251 branch from 0064f51 to a9b67fb Compare April 27, 2021 14:47

nden approved these changes Apr 27, 2021

View reviewed changes

nden merged commit 744c94c into astropy:main Apr 27, 2021

WilliamJamieson deleted the tickets/AL-251 branch April 27, 2021 16:27

pllim mentioned this pull request Apr 27, 2021

Removed unnecessary warning filtering for math_functions #11649

Merged

pllim mentioned this pull request Aug 6, 2021

Follow-up of PR 11548: Improve broadcasting logic #12021

Open

pllim mentioned this pull request Mar 24, 2022

Fix exact comparison causing test failure #12987

Merged

10 tasks

lpsinger mentioned this pull request Aug 21, 2023

MNT: fix incorrect comment in test_inconsistent_input_shapes #15214

Merged

Uh oh!

Uh oh!

Conversation

WilliamJamieson commented Apr 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

pep8speaks commented Apr 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated at 2021-04-27 14:47:34 UTC

Uh oh!

pllim commented Apr 13, 2021

Uh oh!

pllim commented Apr 13, 2021

Uh oh!

WilliamJamieson commented Apr 13, 2021

Uh oh!

WilliamJamieson commented Apr 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcara commented Apr 14, 2021

Uh oh!

WilliamJamieson commented Apr 15, 2021

Uh oh!

WilliamJamieson commented Apr 19, 2021

Uh oh!

WilliamJamieson commented Apr 22, 2021

Uh oh!

perrygreenfield left a comment

Choose a reason for hiding this comment

Uh oh!

WilliamJamieson commented Apr 23, 2021

Uh oh!

nden left a comment

Choose a reason for hiding this comment

Uh oh!

chris-simpson commented Aug 6, 2021

Uh oh!

pllim commented Aug 6, 2021

Uh oh!

pllim commented Aug 6, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

WilliamJamieson commented Apr 13, 2021 •

edited

Loading

pep8speaks commented Apr 13, 2021 •

edited

Loading

WilliamJamieson commented Apr 13, 2021 •

edited

Loading