Skip to content

Comments

fix: separability for nested CompoundModels (swev-id: astropy__astropy-12907)#83

Open
casey-brooks wants to merge 2 commits intoastropy__astropy-12907from
fix/separability-nested
Open

fix: separability for nested CompoundModels (swev-id: astropy__astropy-12907)#83
casey-brooks wants to merge 2 commits intoastropy__astropy-12907from
fix/separability-nested

Conversation

@casey-brooks
Copy link

@casey-brooks casey-brooks commented Dec 18, 2025

Description

  • add regression coverage for CompoundModel nesting
  • fix _cstack to concatenate right coord matrices without dropping structure
  • commits include [skip ci] per instructions to avoid running CI

Reproduction Steps

  1. Plain conjunction (matches expectation):
    from astropy.modeling import models
    from astropy.modeling.separable import separability_matrix
    
    compound = models.Linear1D(10) & models.Linear1D(5)
    print(separability_matrix(compound))
    # expected / observed
    # [[ True False]
    #  [False  True]]
  2. Mixed compound (non-nested) remains correct:
    compound = models.Pix2Sky_TAN() & models.Linear1D(10) & models.Linear1D(5)
    print(separability_matrix(compound))
    # expected / observed
    # [[ True  True False False]
    #  [ True  True False False]
    #  [False False  True False]
    #  [False False False  True]]
  3. Nested compound (pre-fix observed incorrect duplication):
    inner = models.Linear1D(10) & models.Linear1D(5)
    compound = models.Pix2Sky_TAN() & inner
    print(separability_matrix(compound))
    # expected
    # [[ True  True False False]
    #  [ True  True False False]
    #  [False False  True False]
    #  [False False False  True]]
    # observed (before fix)
    # [[ True  True False False]
    #  [ True  True False False]
    #  [False False  True  True]
    #  [False False  True  True]]

Failing Pytest Trace (pre-fix)

______________________ test_separability_nested_compound _______________________

    def test_separability_nested_compound():
        inner = models.Identity(2)
        compound = models.Identity(2) & inner
        expected = np.eye(4, dtype=bool)
>       assert np.array_equal(separability_matrix(compound), expected)
E       assert False
E        +  where False = <function array_equal at 0xffff98d088b0>(array([[ True, False, False, False],
E       [False,  True, False, False],
E       [False, False,  True,  True],
E       [False, False,  True,  True]]), array([[ True, False, False, False],
E       [False,  True, False, False],
E       [False, False,  True, False],
E       [False, False, False,  True]]))
E        +    where <function array_equal at 0xffff98d088b0> = np.array_equal
E        +    and   array([[ True, False, False, False],
E       [False,  True, False, False],
E       [False, False,  True,  True],
E       [False, False,  True,  True]]) = separability_matrix(<CompoundModel()>)

../modeling/tests/test_separable_regressions.py:28: AssertionError

Fixes #66

@casey-brooks
Copy link
Author

Local Validation

  • python3 -m pytest test_separable_regressions.py
    • 4 passed, 0 failed, 0 skipped

@casey-brooks casey-brooks changed the title Fix separability matrix for nested compound models fix: separability for nested CompoundModels (swev-id: astropy__astropy-12907) Dec 18, 2025
@rowan-stein
Copy link
Collaborator

Requesting review:

This PR fixes separability_matrix for nested CompoundModels by preserving the right-hand nested structure in _cstack.

Focus areas:

  • Correctness of _cstack bottom-right block assignment
  • Test coverage adequacy and clarity
  • Consistency with existing modeling/separable conventions

Copy link

@noa-lucent noa-lucent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix swaps the all-ones placeholder in _cstack for the actual right-hand coord matrix, so nested CompoundModels retain their separability structure. The new regression suite exercises the plain, mixed, nested, and deeply-nested cases and matches modeling semantics (fails prior to the fix, passes here). Looks great—thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants