Skip to content

Fix ScalarDistribution returning structured array with ellipsis (Issu… - #20670

Merged
mhvk merged 1 commit into
astropy:mainfrom
Onkar-Raskar:fix-scalar-distribution-ellipsis
Oct 24, 2026
Merged

mhvk merged 1 commit into
astropy:mainfrom
Onkar-Raskar:fix-scalar-distribution-ellipsis

Conversation

@Onkar-Raskar

Copy link
Copy Markdown
Contributor

Closes #20455.

this implements the __getitem__ workaround for ScalarDistribution so that using the ellipsis operator ([...]) correctly returns a Distribution instead of a raw structured ndarray.

I have also added a regression test.

@giles

giles Bot commented Oct 21, 2026

Copy link
Copy Markdown

Hi 👋 and thank you for your contribution! 🙏

This pull request is being closed automatically - but don't worry, this is not the end, and we may re-open it.

For some background, we have recently started seeing a rapid increase in the number of pull requests opened. Some of these are from good-faith humans, but some of which are from autonomous LLM agents or humans using LLMs who do not have a genuine understanding of, or interest in, the project. We are therefore auto-closing pull requests from new contributors, but if you are a good-faith human, we want to make sure your pull request gets considered! So if you would like us to re-open your pull request so that it gets reviewed, you need to do two things:

  1. Add a comment here to explain why you need the changes here to be considered, as in how the bug or missing feature affects your work, or whether this is an issue you have encountered but does not affect you.

  2. Join the astropy slack using this invite link and head over to the #new-contributor-requests channel to introduce yourself and let us know about this pull request

The second step is important, as we may otherwise miss notifications about this pull request.

To be clear, our AI policy does allow the use of LLMs as part of contributions, but we need to see authentic engagement and understanding from humans making the contribution.

Notes for maintainers

If you know this is a genuine user and the PR is is compliance with the AI policy, you may re-open this straight away. After two successful merged pull requests, the user will be added to the allowlist and future PRs will no longer be closed automatically.

This user has not made any pull requests to this repository prior to this one.

In addition, here are some statistics on the user's activity on GitHub:

Last 24 hours Last 7 days
Pull requests opened 1 1
Issues opened 0 0

@giles giles Bot closed this Oct 21, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.

  • Do the proposed changes actually accomplish desired goals?
  • Do the proposed changes follow the Astropy coding guidelines?
  • Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
  • Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
  • Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
  • Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
  • Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
  • Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
  • At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

@Onkar-Raskar

Copy link
Copy Markdown
Contributor Author

Hi, I thought to work on this issue because it seemed like a good issue to begin with, i have test getitem workaround locally and also added regression test and verified that it passes all test. can we get it reopened again ?

@pllim pllim added this to the v8.1.0 milestone Oct 21, 2026
@pllim

pllim commented Oct 21, 2026

Copy link
Copy Markdown
Member

@mhvk , I will let you decide if this is worth re-opening. Thanks, all!

@mhvk mhvk reopened this Oct 22, 2026

@mhvk mhvk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Onkar-Raskar - this looks good.

One request: could you also add a test for the only other case that makes sense with indexing a ScalarDistribution, i.e., with s[()] - that should just return a new instance that has the same content. (Note that this works correctly, so the only thing needed is to add a test for it.)


# Using ellipsis should return a Distribution, not a structured ndarray
result = s[...]
assert isinstance(result, Distribution) No newline at end of file

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Might as well add the line ending.

@mhvk

mhvk commented Oct 22, 2026

Copy link
Copy Markdown
Contributor

p.s. I'll let the tests run with the revised PR; would be good to have pre-commit pass before doing that.

@Onkar-Raskar

Copy link
Copy Markdown
Contributor Author

Ok i will make changes and update you

@Onkar-Raskar
Onkar-Raskar force-pushed the fix-scalar-distribution-ellipsis branch from 6e50169 to 120cbb9 Compare October 24, 2026 17:23
@Onkar-Raskar

Copy link
Copy Markdown
Contributor Author

@mhvk , I have made changes also i squashed all commits to get rid on unnecessary number of commits, can you check it once.

@mhvk mhvk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks! Let's get this in.

@mhvk
mhvk enabled auto-merge October 24, 2026 18:32
@mhvk
mhvk merged commit dbbf2ac into astropy:main Oct 24, 2026
43 of 44 checks passed
@pllim

pllim commented Oct 25, 2026

Copy link
Copy Markdown
Member

@mhvk , why no change log needed here? Can you please clarify? Thanks!

@mhvk

mhvk commented Oct 25, 2026

Copy link
Copy Markdown
Contributor

Mostly because it is a buglet that was only found indirectly - basically the ScalarDistribution class is, in practice, never actually used except to help the repr of a regular Distribution. So, I see this more as a cleanup/refactoring, with no user-visible impact.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ScalarDistribution behaves weirdly

3 participants