Skip to content

[BUG] Fix backward independence routing in Mixture sample#979

Open
ANANYA542 wants to merge 1 commit intosktime:mainfrom
ANANYA542:fix-mixture-independence
Open

[BUG] Fix backward independence routing in Mixture sample#979
ANANYA542 wants to merge 1 commit intosktime:mainfrom
ANANYA542:fix-mixture-independence

Conversation

@ANANYA542
Copy link
Copy Markdown
Contributor

Reference Issues/PRs

fixes #974

What does this implement/fix? Explain your changes.

This PR fixes an issue in Mixture._sample where the indep_rows and indep_cols flags were handled in reverse.

Previously, the implementation set rd_size[...] = 1 when the independence flags were True, which caused np.random.choice to draw a single component and broadcast it across the dimension, making it fully dependent. When the flags were False, the full dimension was retained, resulting in independent sampling instead.

This PR corrects the logic by inverting the condition:

if not indep_rows:
    rd_size[0] = 1
if not indep_cols:
    rd_size[1] = 1

This restores the intended behavior where independence retains full sampling dimensions and dependence enforces broadcasting.

Does your contribution introduce a new dependency? If yes, which one?

No

What should a reviewer concentrate their feedback on?

  • Whether the updated logic correctly reflects the intended behavior of indep_rows and indep_cols
  • Whether the change is consistent with other sampling implementations in the codebase

Did you add any tests for the change?

Tested locally using larger mixture arrays (e.g., 20x2). The output matches expectations:

  • dependent settings produce identical rows
  • independent settings produce varied rows

Any other comments?

A verification screenshot demonstrating the behavior has also been attached for reference.
Screenshot 2026-03-20 at 11 48 05 AM

PR checklist

For all contributions
  • I've added myself to the list of contributors with any new badges I've earned :-)
    How to: add yourself to the all-contributors file in the skpro root directory (not the CONTRIBUTORS.md). Common badges: code - fixing a bug, or adding code logic. doc - writing or improving documentation or docstrings. bug - reporting or diagnosing a bug (get this plus code if you also fixed the bug in the PR).maintenance - CI, test framework, release.
    See here for full badge reference
  • The PR title starts with either [ENH], [MNT], [DOC], or [BUG]. [BUG] - bugfix, [MNT] - CI, test framework, [ENH] - adding or improving code, [DOC] - writing or improving documentation or docstrings.
For new estimators
  • I've added the estimator to the API reference - in docs/source/api_reference/taskname.rst, follow the pattern.
  • I've added one or more illustrative usage examples to the docstring, in a pydocstyle compliant Examples section.
  • If the estimator relies on a soft dependency, I've set the python_dependencies tag and ensured
    dependency isolation, see the estimator dependencies guide.

Copy link
Copy Markdown
Collaborator

@fkiraly fkiraly left a comment

Choose a reason for hiding this comment

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

Can you explain why it is correct this way round?

@fkiraly fkiraly added bug module:probability&simulation probability distributions and simulators labels Mar 21, 2026
@ANANYA542
Copy link
Copy Markdown
Contributor Author

@fkiraly —thanks for reviewing
this comes down to how rd_size controls the shape of the component draw before it gets broadcast.If a dimension is set to 1, np.random.choice draws a single component which then gets broadcast across that axis, making it dependent.
In the current implementation this happens when indep_rows=True, so all rows end up sharing the same component, which is the opposite of what we want.Flipping it to if not indep_rows keeps the full dimension when independence is requested, and only collapses it when dependence is intended. Same applies to columns as well.

@ANANYA542 ANANYA542 requested a review from fkiraly March 22, 2026 11:35
@ANANYA542
Copy link
Copy Markdown
Contributor Author

Hi @fkiraly — just following up on this. I’ve addressed your question above and clarified the reasoning behind the change.Could you please take another look when you have time and let me know if any further clarification or changes are needed?

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

Labels

bug module:probability&simulation probability distributions and simulators

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Mixture._sample handles indep_cols and indep_rows backward, destroying independence assumptions

2 participants