Repository navigation
Shape changing mixin - #4123
Shape changing mixin#4123
Conversation
|
Note that the sphinx build fails, since somehow the references to EDIT: Oddly, I cannot reproduce this locally. |
There was a problem hiding this comment.
I guess you can make _replicate an abstractmethod?
There was a problem hiding this comment.
Yes, I thought about making it an ABC as well. It has the advantage that I could also define ndim and shape as abstract properties, since these are needed for some of the methods.
|
As for the docs build, I think there's a setting somewhere you can tweak for the API docs for whether or not class methods should be inherited. I need to check exactly where you change that... |
|
@embray - I tried changing |
|
You'll have to show me what you mean. |
|
Fair enough -- see the branch at https://github.com/astropy/astropy/compare/master...mhvk:shape-changing-mixin-abc?expand=1 With that, |
|
Oh I see what you're saying. For that to work you have to make |
|
@embray - After a few more trials, it seems it is indeed as simple as making |
|
Yeah, the problem with that now is that everything that uses But I'm fine with seeing how that goes for now. |
|
@embray - I moved the |
|
Okay, sounds good. Odd about the docs. I'll see if I can reproduce that. |
|
@eteq, @taldcroft, @astrofrog - I think enabling multidimensional coordinates and allowing their shape to change would be nice to get in for 1.1, but I don't think I'll get around to adding test cases. Any of you have a change? Obviously, one can use those in @embray - any luck with figuring out the reason the docs don't link correctly? |
|
@mhvk - sorry, I'm way behind just trying to get all my own PRs and assigned PRs wrapped up. |
|
@mhvk - I see your point and can try to give it a shoot (especially given its been done for some time...). Just so we're clear, it's basically |
|
@eteq - yes, it is. Not sure one would need to be as thorough as that one, given that the same code is used, but I think it is especially important to check that view and copies are done appropriately (e.g., for the representations, I needed to add |
|
sadly, I haven't had time to get tests in on this, and it appears to now need a rebase. So I think we're going to have to triage it to 1.2 😢 |
f6fa4c2 to
fcc32c2
Compare
63540e0 to
34f4c29
Compare
|
A minor comment: With numpy arrays, it is possible to set the shape attribute to change the shape if that wouldn't change the memory layout of the underlying array (see http://docs.scipy.org/doc/numpy/reference/generated/numpy.ndarray.shape.html). Perhaps that isn't a guarantee we want to keep, but I use |
|
@alexrudy - I did not yet implement shape-setting for coordinates, as it was not something that was as easy to do with the mixin class proposed here as the ndarray methods, which all rely on a |
| return self._replicate('squeeze', *args, **kwargs) | ||
|
|
||
| def take(self, indices, axis=None, mode='raise'): | ||
| """Return a new instance formed from the elements the given indices. |
There was a problem hiding this comment.
'from the elements at the given indices'
|
Hi @mhvk. I think this looks very nice, though it took me a while to wrap my head around the metaclass magic... A couple of extra comments:
...
/usr/local/lib/python3.5/site-packages/astropy/coordinates/builtin_frames/icrs_cirs_transforms.py in gcrs_to_icrs(gcrs_coo, icrs_frame)
171 astrom = erfa.apcs13(jd1, jd2, pv)
172
--> 173 i_ra, i_dec = aticq(gcrs_ra, gcrs_dec, astrom)
174
175 if gcrs_coo.data.get_name() == 'unitspherical' or gcrs_coo.data.to_cartesian().x.unit == u.one:
/usr/local/lib/python3.5/site-packages/astropy/coordinates/builtin_frames/utils.py in aticq(ri, di, astrom)
184
185 # Bias-precession-nutation, giving GCRS proper direction.
--> 186 ppr = erfa.trxp(astrom['bpn'], pos)
187
188 # Aberration, giving GCRS natural direction
/usr/local/lib/python3.5/site-packages/astropy/_erfa/core.py in trxp(r, p)
24006
24007 #Create the output array, based on the broadcasted shape, adding the generated dimensions if needed
> 24008 broadcast = numpy.broadcast(numpy.int32(0.0), numpy.int32(0.0), r_in[...,0,0], p_in[...,0])
24009 trp_out = numpy.empty(broadcast.shape + (3,), dtype=numpy.double)
24010
ValueError: shape mismatch: objects cannot be broadcast to a single shapeIt might be best to merge this PR, and create an issue for that problem to be fixed in a separate PR, especially as this doesn't work in the master branch either. |
Also ensure shape-changing operations return views rather than copies.
Also move the ABCMeta inheritance to where it is needed, commenting on on why it is there.
fdfef92 to
15106c1
Compare
|
@StuartLittlefair - indeed, it is not necessary to redefine On the |
|
@mhvk - yes it's currently broken in master too. I'll raise a new issue now... |
91fa8c1 to
caa6f93
Compare
|
@StuartLittlefair - OK, I removed the unnecessary redefinitions of I liked your simple It should be ready to go in now! |
|
@eteq, @taldcroft, @StuartLittlefair - any last comments on the shape-changing PR? |
|
Nothing more to add from me @mhvk! |
7 participants
EDIT: this is now a simplied follow-up of #4096, where a new
ShapedLikeNDArraymixin class is created, which is used inTimeand in the various coordinate representations. To also use it in frames and coordinates will require that the frame and coordinate attributes are properly broadcast at creating time.OLD TEXT FOR REFERENCE
This is a first cut at #4096, where following @embray's suggestion the shape-changing methods from
Timeare split off to a newShapedLikeNDArraymixin class and used inCoordinates. With it,Comments very welcome (as are PRs with test cases; I'd prefer not to have to write them myself; was boring enough for
Time!). cc: @taldcroft, @eteq.Note that there are no test cases yet, though it seems all regular tests pass (which means that
__getitem__is correctly done). A̶l̶s̶o̶,̶ ̶a̶ ̶p̶r̶o̶b̶l̶e̶m̶ ̶i̶s̶ ̶t̶h̶a̶t̶ ̶̶s̶c̶.̶T̶̶ ̶d̶o̶e̶s̶ ̶n̶o̶t̶ ̶w̶o̶r̶k̶ (EDIT: this is solved by the last commit)