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.

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.

I don't know if @WilliamJamieson changed any tests so the results adhered to his logic.

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).

There seems to be uncertainty over what the correct broadcasting behaviour should be and so perhaps this needs to be resolved first.

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 this is the logic:

If all the inputs are scalars, then the output should be a scalar (if n_outputs==1) or a tuple (if n_outputs>1)

If any of the inputs are iterables, then all iterables must be the same length

If there are also some scalars, then these should be broadcast into iterables of the same length as the other iterables

The output should be an array (if n_outputs==1) or a tuple of arrays (if n_outputs>1), all arrays being the same length as the input iterables

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:

Due to the fragile nature of what is currently implemented it is possible that this logic could break existing use cases for astropy modeling. Hence, I believe a more comprehensive survey of how input broadcasting is used should be performed. Depreciation warnings (and temporary special handling) may need to be included for currently working cases, which the new logic would break.
I believe that we should be extremely considered when deciding what logic will be used in the end. I would like to explicitly cover as many (hopefully all) possibilities for input broadcasting as possible, as either explicitly allowed or disallowed (with reasonable explanations as error messages).

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.

Uh oh!

Uh oh!

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

Description

Metadata

Metadata

Assignees

Labels

Type

Fields

Projects

Milestone

Relationships

Development

Issue actions