Repository navigation
Follow-up of PR 11548: Improve broadcasting logic #12021
Description
#11548 was milestoned for 4.2.2 . This issue was created based on response from @chris-simpson in #11548 (comment) .
cc @WilliamJamieson , @perrygreenfield , and @nden
Activity
nstarman commentedon Aug 6, 2021
@pllim do you mean #11548, not #11558 in the above comment?
pllim commentedon Aug 6, 2021
Whoops, sorry. I fixed it.
chris-simpson commentedon Aug 6, 2021
First, I don't think #11548 breaks anything that previously worked -- all the examples I gave in my comment fail under v4.2. It also does fix the specific failure in #10170 although it turns out I gave too simple an example there (
n_outputs=1). So I don't see any obvious reason to revert it, as it appears to be an improvement, even if it's not a complete fix. However, as I noted at the start of my comment to #11548 there seems to be some uncertainty about what the correct behaviour should be for broadcasting, and therefore I don't know if @WilliamJamieson changed any tests so the results adhered to his logic.There seems to be uncertainty over what the correct broadcasting behaviour should be and so perhaps this needs to be resolved first. I think this is the logic:
n_outputs==1) or a tuple (ifn_outputs>1)n_outputs==1) or a tuple of arrays (ifn_outputs>1), all arrays being the same length as the input iterablesThis logic will also result in
fix_inputsworking the way I think it's supposed to without the need for any specific code.I haven't looked in any detail at the code that deals with broadcasting so I don't think it will be efficient for me to try to write a PR to enact the logic described above if everyone agrees that's what should happen (sorry if this sounds like I'm passing the buck). Perhaps the question to be answered is whether #11548 represents progress on the path to this behaviour or whether it's gone down a side alley and the previous code is a better starting point.
WilliamJamieson commentedon Aug 9, 2021
This is good to hear! I tried very hard to not break anything that was previously working with #11548. I agree that there is no need to revert #11548 at this time.
If one looks closely at the test files changed by #11548, then one can see that all the changes were adding more test coverage for the known broadcasting issues, with the exception of one change to this test. This change was necessary because my changes meant that row and column vector inputs would be now be broadcast to a common row (length) by common (length) shaped inputs instead of raising an error (if I remember correctly this was part of fixes for issues #9953 and #11060).
This is essentially the main issue with input/output shapes and broadcasting. Astropy modeling's documentation does not explicitly describe what input/output shapes and broadcasting logic it will and will not support. I think that we should add a section to the documentation which details the input/output shapes/broadcasting which are supported. This uncertainty has resulted in the current astropy methods for handling input shapes and broadcasting being quite fragile (and poorly documented), as your newly reported bug appears to have exposed again.
As for your suggestions on what logic/interface to use:
I think these are a good start on what logic astropy modeling should employing. I however hesitate to make the decision that these should be the logic used by astropy for a few reasons:
For whatever broadcasting logic we decide on for input/output interface (if we decide to do so), I think we should develop a comprehensive set of test cases which test as many allowed and disallowed input/output possibilities as possible under this logic. These tests should be to most (if not all) of the astropy models which are currently provided.
[-]Revert PR 11548 ?[/-][+]Follow-up of PR 11548: Improve broadcasting logic[/+]pllim commentedon Aug 9, 2021
Okay. I updated the title. Feel free to edit the title (again) if I misunderstood.