Repository navigation
Fix ScalarDistribution returning structured array with ellipsis (Issu… - #20670
Conversation
|
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:
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 maintainersIf 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:
|
|
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.
|
|
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 ? |
|
@mhvk , I will let you decide if this is worth re-opening. Thanks, all! |
mhvk
left a comment
There was a problem hiding this comment.
@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 |
There was a problem hiding this comment.
Might as well add the line ending.
|
p.s. I'll let the tests run with the revised PR; would be good to have pre-commit pass before doing that. |
|
Ok i will make changes and update you |
6e50169 to
120cbb9
Compare
|
@mhvk , I have made changes also i squashed all commits to get rid on unnecessary number of commits, can you check it once. |
|
@mhvk , why no change log needed here? Can you please clarify? Thanks! |
|
Mostly because it is a buglet that was only found indirectly - basically the |
Closes #20455.
this implements the
__getitem__workaround forScalarDistributionso that using the ellipsis operator ([...]) correctly returns aDistributioninstead of a raw structuredndarray.I have also added a regression test.