cpirich / astropy

Astronomy and astrophysics core library
https://www.astropy.org
BSD 3-Clause "New" or "Revised" License
0 stars 0 forks source link

JACoB PR for Issue Modeling's `separability_matrix` does not compute separability correctly for nested CompoundModels #15

Open jacob-ai-bot[bot] opened 3 weeks ago

jacob-ai-bot[bot] commented 3 weeks ago

Summary:

Consider the following model:

from astropy.modeling import models as m
from astropy.modeling.separable import separability_matrix

cm = m.Linear1D(10) & m.Linear1D(5)

It's separability matrix as you might expect is a diagonal:

>>> separability_matrix(cm)
array([[ True, False],
       [False, True]])

If I make the model more complex:

>>> separability_matrix(m.Pix2Sky_TAN() & m.Linear1D(10) & m.Linear1D(5))
array([[ True, True, False, False],
       [ True, True, False, False],
       [False, False, True, False],
       [False, False, False, True]])

The output matrix is again, as expected, the outputs and inputs to the linear models are separable and independent of each other.

If however, I nest these compound models:

>>> separability_matrix(m.Pix2Sky_TAN() & cm)
array([[ True, True, False, False],
       [ True, True, False, False],
       [False, False, True, True],
       [False, False, True, True]])

Suddenly the inputs and outputs are no longer separable?

This feels like a bug to me, but I might be missing something?

@jacob-ai-bot --skip-build --branch issue_12906 https://github.com/astropy/astropy/issues/12906

Plan:

Step 1: Edit /astropy/modeling/separable.py

Task: Fix separability matrix computation for nested CompoundModels

Instructions: Modify the _separable function to correctly handle nested CompoundModel instances. The function should recursively traverse the compound model tree and apply the appropriate operator function from the _operators dictionary based on the operator of each CompoundModel. Ensure that the separability matrix is computed correctly for each level of nesting, taking into account the inputs and outputs of each component model. The fix should address the issue where the separability matrix incorrectly indicates non-separability for nested compound models when the component models are actually separable.

Exit Criteria: The separability_matrix function should return the correct separability matrix for nested CompoundModel instances, as demonstrated by the example in the issue description. Specifically, for the model m.Pix2Sky_TAN() & cm, where cm = m.Linear1D(10) & m.Linear1D(5), the function should return a diagonal matrix indicating separability.

Step 2: Edit /astropy/modeling/tests/test_separable.py

Task: Update tests for nested CompoundModels

Instructions: Add or modify test cases in /astropy/modeling/tests/test_separable.py to specifically test the separability matrix computation for nested CompoundModel instances. Include test cases that cover different combinations of operators and models, ensuring that the separability_matrix function returns the correct results in all scenarios, particularly for the case described in the issue where nested linear models combined with Pix2Sky_TAN resulted in an incorrect separability matrix. Ensure that the tests cover both separable and non-separable nested compound models.

Exit Criteria: All new and existing tests in /astropy/modeling/tests/test_separable.py should pass, demonstrating the correct computation of separability matrices for nested CompoundModel instances.

jacob-ai-bot[bot] commented 3 weeks ago

Hello human! 👋

This PR was created by JACoB to address the issue Modeling's separability_matrix does not compute separability correctly for nested CompoundModels

Next Steps

  1. Please review the PR carefully. Auto-generated code can and will contain subtle bugs and mistakes.

  2. If you identify code that needs to be changed, please reject the PR with a specific reason. Be as detailed as possible in your comments. JACoB will take these comments, make changes to the code and push up changes. Please note that this process will take a few minutes.

  3. Once the code looks good, approve the PR and merge the code.