Repository navigation
Conversation
As part of this, define _get_distribution_cls in analogy to what is done in Masked._get_masked_cls. This is used both in the initializer and in .view().
Arguably, this is more logical, since we want the array to look like a regular array as much as possible. It is also required to get structured dtypes to work with Quantity.
mhvk
added
Enhancement
uncertainty
API change
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.
|
Contributor
|
👋 Thank you for your draft pull request! Do you know that you can use |
mhvk
force-pushed
the
distribution-override-dtype-structured
branch
from
September 8, 2023 15:19
80f3060 to
4f576c4
Compare
nstarman
reviewed
Sep 9, 2023
nstarman
left a comment
nstarman
left a comment
Member
There was a problem hiding this comment.
There's some deep numpy foo, so this is not a full review. I just glanced through. From the tests, everything looks good
mhvk
force-pushed
the
distribution-override-dtype-structured
branch
from
September 11, 2023 08:40
4f576c4 to
cfec396
Compare
This involves a second indirection in the actual dtype of the array, which now has a "samples" entry that itself consists of a n_samples "sample" entries. This is required to ensure we can get specific fields out of a distribution with a structured dtype.
mhvk
force-pushed
the
distribution-override-dtype-structured
branch
2 times, most recently
from
September 11, 2023 09:59
0797aa1 to
1b353ac
Compare
With the new sample indirection, it becomes possible, by using stride trickery to ensure that, e.g., viewing a complex number as two floats will give the correct new shape.
mhvk
force-pushed
the
distribution-override-dtype-structured
branch
from
September 11, 2023 10:05
1b353ac to
8cb8452
Compare
1 task done
Contributor
Author
|
#15304, which includes this, was reviewed as a whole and will get merged, so am closing this one. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This pull request is to allow structured data types for
Distribution, so that classes likeEarthLocationcan be made intoDistributionas well (though that will need some changes inEarthLocationitself).To make this possible, an API change was necessary, where the
.dtypeexposed by the distribution now equals the.dtypeof the samples (rather than the structured dtype used internally). I could not getQuantityDistributionto work without changes toQuantityandStructuredUnitthat I felt were not warranted. And arguably, it is more logical for the distribution to look as much as possible like a regular array, so to expose the regulardtype.Note that this builds on #15295; it may be best to get that in first. Otherwise, reviewing by commit may be helpful.