Repository navigation
Allow Quantity to be initialized with a Distribution - #11210
Conversation
1660f07 to
2a64d5a
Compare
2a64d5a to
ff1c0b7
Compare
|
@eteq, @astrofrog - would one of you have time to review this (or suggest someone else)? I'd like to try to get the |
ff1c0b7 to
1c17aa5
Compare
|
@eteq, @astrofrog - Gentle ping - quite simple PR and review would be nice... |
1c17aa5 to
f0372fe
Compare
|
@eteq - a promised ping - would be good to have this reviewed. I just rebased to use the new changelog fragment format. |
| return super().view(dtype) | ||
| else: | ||
| result = self.distribution.view(dtype) | ||
| return Distribution(result) |
There was a problem hiding this comment.
Nitpick and feel free to ignore. I'm just thinking of McCabe complexity. I endeavor to structure my personal code to minimize the complexity score and reducing the number of return statements is an easy couple points. I should never have turned on that checker! 😆
There was a problem hiding this comment.
Numpy's strange choice to allow one to omit dtype and just pass in a class remains a bit annoying... I've made it slightly simpler (and found a bug on the way, so good!).
f0372fe to
9e8c558
Compare
eteq
left a comment
eteq
left a comment
There was a problem hiding this comment.
LGTM broad-brush! I have a few minor inline comments, but nothing major!
I also noticed @astrofrog's #11210 (review) - @astrofrog, were you thinking @mhvk should do that in this PR, or that we should do that separately?
| with pytest.raises(ValueError, match='just a new dtype'): | ||
| ad.view(np.dtype('f8')) |
There was a problem hiding this comment.
This is confusing to me. Is it that this works for a regular Angle but not one that uses Distribution? Or it doesn't work for any angle? If it's the former I think it's probably a bug (which we can fix later to not hold up this PR, but should make an issue and add a comment here) whereas if it's the latter this should probably go in the regular Angle tests.
There was a problem hiding this comment.
Indeed, it works for Angle but cannot very easily work for any Distribution (since it has a structured dtype) - also just don't quite know what to do with things that change the number of elements (say, .view('f4')) - in practice, this basically never happens...
Anyway, I'll add a comment.
There was a problem hiding this comment.
Now added a comment and restructured the .view code as well.
|
Follow up :) |
Also ensure that subclasses passes, independent of their order.
This allows Quantity to initialize from Distribution instances, ensuring that will return a QuantityDistribution.
f33ea07 to
48e6caf
Compare
4 participants
This is really a spin-off from trying to get a general
Maskedclass to work in #11127, where I wanted to ensuring thatQuantity(masked, unit)gave aMaskedQuantity. But the changes I made suggestedQuantity(distribution, unit)should also give aQuantityDistribution- which indeed worked! I split the required changes out of #11127, since it is likely much easier to review here (and being able to dodistribution * unitis nice!).